open-feature / spec

OpenFeature specification
https://openfeature.dev
Apache License 2.0
639 stars 34 forks source link

Don't allow exceptions during client creation #44

Closed moredip closed 2 years ago

moredip commented 2 years ago

I just noticed that the current draft spec has:

Requirement 1.19 No methods, functions, or operations on the client should ever throw exceptions, or otherwise abnormally terminate. Flag evaluation calls must always return the default value in the event of abnormal execution. Exceptions include functions or methods for the purposes for configuration or setup.

I understand the motivation for never throwing exceptions, but I think we're failing to meet the spirit of this for client-side JS if we allow exceptions when creating a client, because of the unique lifecycle of browser-based apps. For a client-side app the client is often created just before an evaluation is done, so an exception thrown during client creation is pretty much just as disruptive as an exception when you call a method.

Because of this, I would propose that the spec not allow exceptions during client creation.

agentgonzo commented 2 years ago

I imagine the normal setup will involve creating a client/provider which includes passing some kind of ID to the provider (application/team/environnment ID/key etc). I would imagine that the provider will do some initialization etc.

What do you propose to do if the coder provides an invalid key/id etc in this case? To me, throwing an error is the most obvious thing.

toddbaert commented 2 years ago

I understand the motivation for never throwing exceptions, but I think we're failing to meet the spirit of this for client-side JS if we allow exceptions when creating a client, because of the unique lifecycle of browser-based apps. For a client-side app the client is often created just before an evaluation is done, so an exception thrown during client creation is pretty much just as disruptive as an exception when you call a method.

@moredip This makes sense. I agree.

Even in some toy examples, I've created clients per-request on the server, and I think that's a pattern we want to support.

I imagine the normal setup will involve creating a client/provider which includes passing some kind of ID to the provider (application/team/environnment ID/key etc). I would imagine that the provider will do some initialization etc.

What do you propose to do if the coder provides an invalid key/id etc in this case? To me, throwing an error is the most obvious thing.

The provider registration happens on the global API singleton, not on the client. The client is a lightweight class for doing evaluation with the previously registered, global provider, thats generated from the global API singleton. I think that provider registration is where the sort of exception you're mentioning would occur.

// this can probably throw
var myProvider = new MyProvider(SUPER_SECRET_APP KEY)

// this can probably throw too
openfeature.registerProvider(myProvider);

// this should probably not
var client = openfeature.getClient(...)
toddbaert commented 2 years ago

This also might relate to a question that's come up a few times: do we want to expose events (specifically readiness events) from the provider interface? Providers could sidestep this by requiring SDKs that have a readiness event simply be ready BEFORE passing them into the provider. For instance, the CloudbeesProvider may require an already-initialized Cloudbees SDK instance. Not sure.

toddbaert commented 2 years ago

I guess the only question here is if we should specifically add a requirement that .getClient() calls never throw. I think it's probably a good thing to do.