Skip to content

feat: eventing poc#316

Closed
toddbaert wants to merge 1 commit intomainfrom
feat/event-streaming
Closed

feat: eventing poc#316
toddbaert wants to merge 1 commit intomainfrom
feat/event-streaming

Conversation

@toddbaert
Copy link
Copy Markdown
Member

@toddbaert toddbaert commented Nov 4, 2022 •
edited
Loading

A PoC for eventing, meant to be used with the experimental flagd-web provider.

Specifically:

  • adds an addHandler() method to the client which can be passed an event type (PROVIDER_READY, PROVIDER_ERROR,PROVIDER_CONFIGURATION_CHANGED, PROVIDER_SHUTDOWN and a function to run when that event is fired)
  • adds a new CACHED default reason
  • adds a new EventProvider interface, which providers can OPTIONALLY implement to fire events.

@toddbaert toddbaert requested a review from beeme1mr as a code owner November 4, 2022 02:09
@toddbaert toddbaert marked this pull request as draft November 4, 2022 02:09
@toddbaert toddbaert force-pushed the feat/event-streaming branch from 57ddf2a to 1433ffe Compare November 4, 2022 02:10
toddbaert
toddbaert commented Nov 4, 2022
View reviewed changes
src/types.ts Outdated
Comment on lines +5 to +8
export interface ProviderEventing {
events?: EventEmitter;
ready?: boolean;
}
Copy link
Copy Markdown
Member Author

@toddbaert toddbaert Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take the time to document all these types if this looks good to everyone.

@toddbaert toddbaert force-pushed the feat/event-streaming branch 3 times, most recently from 955aaa9 to e490a9f Compare November 7, 2022 20:09
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 7, 2022 •
edited
Loading

Codecov Report

Merging #316 (9d746fa) into main (3acc001) will decrease coverage by 0.59%.
The diff coverage is 89.15%.

@@            Coverage Diff             @@
##             main     #316      +/-   ##
==========================================
- Coverage   99.67%   99.08%   -0.60%     
==========================================
  Files          15       15              
  Lines        1236     1307      +71     
  Branches       97      104       +7     
==========================================
+ Hits         1232     1295      +63     
- Misses          4       12       +8
Impacted Files Coverage Δ
src/client.ts 97.04% <75.67%> (-2.96%) ⬇️
src/open-feature.ts 100.00% <100.00%> (+1.41%) ⬆️
src/types.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@toddbaert toddbaert force-pushed the feat/event-streaming branch 2 times, most recently from 13b7ee7 to 55bf085 Compare November 8, 2022 20:03
toddbaert
toddbaert commented Nov 10, 2022
View reviewed changes
src/types.ts
Comment on lines +5 to +8
export interface EventProvider {
readonly events: EventEmitter;
readonly ready: boolean;
}
Copy link
Copy Markdown
Member Author

@toddbaert toddbaert Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Providers can optionally implement this (backwards compatible) to support events.

All they must do is mark themselves as ready with the boolean, and call events.emit(xxx) to support events.

@toddbaert toddbaert force-pushed the feat/event-streaming branch 3 times, most recently from a14c823 to 61f34e1 Compare November 14, 2022 17:41
@toddbaert toddbaert mentioned this pull request Nov 14, 2022
Merged
@toddbaert toddbaert mentioned this pull request Nov 29, 2022
Closed
@toddbaert toddbaert force-pushed the feat/event-streaming branch from 61f34e1 to 7735bda Compare December 13, 2022 01:33
@toddbaert
feat: eventing poc
86e7e17 
Signed-off-by: Todd Baert <[email protected]>
@toddbaert toddbaert force-pushed the feat/event-streaming branch from 7735bda to 86e7e17 Compare December 13, 2022 01:36
@toddbaert
Copy link
Copy Markdown
Member Author

toddbaert commented Dec 13, 2022

/publish

@kinyoklion
Copy link
Copy Markdown
Member

kinyoklion commented Feb 15, 2023

Using EventEmitter can be somewhat annoying if you are working on non-node platforms. I am not sure if EventTarget is any better in that regard. You can of course, but the more polyfills people need the more annoying things are. Sometimes it is better to just mirror some of that API.

@toddbaert
Copy link
Copy Markdown
Member Author

toddbaert commented Feb 21, 2023

Using EventEmitter can be somewhat annoying if you are working on non-node platforms. I am not sure if EventTarget is any better in that regard. You can of course, but the more polyfills people need the more annoying things are. Sometimes it is better to just mirror some of that API.

Yes, for web, we will probably have a completely separate SDK. To minimize dependencies, in my web SDK PoC, I've used window.dispatchEvent (see here). I've never really loved the ergonomics of that API, bit it works well-enough and I think reducing the need for polyfills is worth it.

@toddbaert
Copy link
Copy Markdown
Member Author

toddbaert commented Mar 13, 2023

This is implemented in the experimental web-sdk.

@toddbaert toddbaert closed this Mar 13, 2023
lukas-reining pushed a commit that referenced this pull request Jun 29, 2023
@github-actions
chore(main): release go-feature-flag-provider 0.5.6 (#316)
c903e0f 
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@beeme1mr beeme1mr Awaiting requested review from beeme1mr

@james-milligan james-milligan Awaiting requested review from james-milligan

@skyerus skyerus Awaiting requested review from skyerus

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@toddbaert @codecov-commenter @kinyoklion