Skip to content
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

Provider to client mappings #56

Merged
merged 4 commits into from
Apr 27, 2023
Merged

Provider to client mappings #56

merged 4 commits into from
Apr 27, 2023

Conversation

justinabrahms
Copy link
Member

refs: #49

Copy link
Member

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

Quick feedback on the format, you're file is not finishing by .md so it does not format well in Github.

@beeme1mr
Copy link
Member

@justinabrahms how would you feel about something like this?

import { OpenFeature } from '@openfeature/js-sdk';

OpenFeature.setProvider(new MyGlobalProvider());

const client = OpenFeature.getClient();
client.overrideProvider(new MyClientProvider());

The idea would be to only allow a single global provider at a time but allow clients to optionally set a new provider that only affects themselves.

@justinabrahms
Copy link
Member Author

justinabrahms commented Mar 17, 2023

@beeme1mr A different way to say that:

  1. libraries must never create a client. They must be passed a client.
  2. If you need to juggle multiple providers (e.g. flagsmith for some stuff, flagd for others, file-based for a third), you will need to juggle multiple clients.

I don't love either of those.

EDIT: Just realized that my solution does item 2 as well. So even if I don't love it.. it's what we got. The big question is item 1, then.

Copy link
Member

@tcarrio tcarrio left a comment

Choose a reason for hiding this comment

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

All good here. Brings utility to the existing named clients (but also will be a breaking change, just to note).

@justinabrahms
Copy link
Member Author

All good here. Brings utility to the existing named clients (but also will be a breaking change, just to note).

My intent would be to still allow setProvider(provider) to set the default one, which should keep it backwards compatible, yes?

@beeme1mr
Copy link
Member

Hey @justinabrahms, would you mind putting together a quick prototype when you have a moment? That may be a good way to move this OFEP along and would make updating the spec straightforward.

@justinabrahms
Copy link
Member Author

Here's a prototype: open-feature/java-sdk#388

@toddbaert
Copy link
Member

Here's a prototype: open-feature/java-sdk#388

Thanks @justinabrahms I'll take a look at this tomorrow!

@toddbaert
Copy link
Member

toddbaert commented Apr 25, 2023

Here's a prototype: open-feature/java-sdk#388

@justinabrahms This makes sense to me. I have a couple thoughts/questions:

  • How do you feel about using "scope" to refer to the "name" for a client. So getProviderForClientOrDefault(String name) would change to getProviderForScopeOrDefault(String scope)? I know it's just naming, but I think referring to this as a "scope" adds clarity, and even allows us to future proof things a bit; there may be other things we want to "scope" in this way. See earlier discussion here. We could add this term to our documentation and glossary.
  • I suppose initialization and events (both not yet completely spec'd out) would be scoped similarly? So change events emitted in a particular provider only impact clients in that provider scope? and setting a new provider for a given scope would shut down the previous one?
  • Would API shutdown (discussed here) still be global? Would there be a need to shutdown only certain providers? I'm leaning towards no, since I think the shutdown is really only useful for terminating the application, in which case all providers should probably be stopped.

cc @thiyagu06 @tcarrio @beeme1mr @thomaspoignant

@justinabrahms
Copy link
Member Author

@toddbaert I dislike using the term 'scope' for the client name. It introduces another term, which I think confuses things. What we're really talking about is binding a client to a specific provider. It's not some "scoping key" that is being bound. It's the named client.

I would expect that initialization would happen for any provider that's registered. I'm unclear about how events would be supported. One option would be a single event stream with a parameter of what is effected or we could have multiple event streams. I don't feel strongly.

I think API shutdown would be global.

Signed-off-by: Justin Abrahms <jabrahms@ebay.com>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <jabrahms@ebay.com>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <jabrahms@ebay.com>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
@thomaspoignant
Copy link
Member

I can agree with @justinabrahms I don't really like the word scope in this context it can be misleading and it is not completely obvious that we are talking about the client name.

@toddbaert
Copy link
Member

I would expect that initialization would happen for any provider that's registered. I'm unclear about how events would be supported. One option would be a single event stream with a parameter of what is effected or we could have multiple event streams. I don't feel strongly.

@justinabrahms my main concern was to confirm that if provider associated with client X gets a "flags change" event, only client X would get that event, right?

@justinabrahms
Copy link
Member Author

@toddbaert I'm not fully up to speed on the events stuff. I don't think there's a technical reason why the thing you're saying can't work though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants