Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Client provider guidelines #14
base: main
Are you sure you want to change the base?
feat: Client provider guidelines #14
Changes from 23 commits
3702d36
34f336a
dbdd5f4
14eaec1
667bb96
e4fa8a3
680d546
4a8a859
175abb3
25d6fbe
f01598f
7f735b2
af3beac
14ac393
4cecfc5
87d4238
0038810
3008500
f3ec588
2522810
ad36231
4a3f70a
961f3c7
69b6f5b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit torn here wether my thinking is going to deep or the use case is too fringe but bare with me:
Some applications have strict rules or goals of how fast they need to be able to show their initial screen. These apps may not have time (or do not want) to wait for network requests to finish before considering a feature flagging SDK to be initialised.
Obviously there are many ways one can solve this, but we have written mobile providers with disk persistence where we then can pass a
FetchingStrategy
to the constructor where the app owner can decide wether or not they want theinitialize()
function to block until fresh data is available or simply load any previous data from disk (and then refresh from the backend in the background).Firebase Remote Config explains the concepts here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of various initialization strategies and the link to Firebase Remote Config is a great resource.
Perhaps one way to implement that would be to put this section under a "Fetch on load" strategy. We could start with a single strategy (and perhaps make it the default) but that would give us the ability to add additional strategies to support specific use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting point here. I see the value of having multiple fetching strategies but I am not sure how to frame that into this document.
I see 2 options:
Do you have any preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking that it might be most reasonable to ignore my comment about this and go ahead with a blocking network call as part of the initialize function in order to not block this document.
If we believe that the topic warrants additional discussion we can open an issue to discuss it there and amend the spec or add an appendix. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this step be optional? The provider should be able to work without having to call the configuration API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also my general feeling, that this should be an opt-in feature as @beeme1mr says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was more thinking toward something that can be enabled by default and opt-out on demand.
See #14 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to leave the
we
out and talk in third person about the provider.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know which error we want to throw here? I think it would be nice to specify here.
Maybe we can even have a new Error type, the case that a system does not support a type happens pretty often. (independently from OFREP)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is true, should we add that to the OF spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh yes I think we should add it to the OF spec.
I could open a PR next days, what do you think @thomaspoignant ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be awesome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is excellent!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the current behavior in the OFREP web provider.
https://github.com/open-feature/js-sdk-contrib/blob/main/libs/providers/ofrep-web/src/lib/ofrep-web-provider.ts#L90
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OFREP web provider by default is doing polling.
You have a default value set if no
pollingInterval
is provided during the initialization.https://github.com/open-feature/js-sdk-contrib/blob/c00f998227f55ca5a64b01d039ab6ec2aff18f7c/libs/providers/ofrep-web/src/lib/ofrep-web-provider.ts#L78