[BUG] Web SDK hooks can return a promise?

[toddbaert]

Both the web and server SDK use a hook interface defined in /shared aka core.
In web, the hook calls are not awaited, so the promise is somewhat useless.

I'm fairly certain this is a bug. We can allow the server to return both promises and non, but the web SDK cannot return Promises in hooks because it doesn't await them and has a synchronous API.

cc @beeme1mr @lukas-reining

[lukas-reining]

Ya, and after having a look I also found that the "hook modified context" is not merged into the flag evaluation context, while it is in the server SDK. In the tests we have a test checking for evaluationContext must be mutable and EvaluationContext must be passed to next "before" hook but not for the client that the before hooks change the context.
I could fix that if we decide that this is really a bug.

[toddbaert]

Ya, and after having a look I also found that the "hook modified context" is not merged into the flag evaluation context, while it is in the server SDK. In the tests we have a test checking for evaluationContext must be mutable and EvaluationContext must be passed to next "before" hook but not for the client that the before hooks change the context. I could fix that if we decide that this is really a bug.

I don't think it's valid that the before hooks in the client change the context in the client implementations. The static context should only be change via the api.setContext method, which is reconciles in the on context changed handler; otherwise things will get mismatched between the context and the reconciled state in the provider. We should probably put 3.2.3 under the same condition as the previous 2 points. Does this make sense to you as well, @beeme1mr ?

[beeme1mr]

Yes, that makes sense to me. Hooks should not be able to modify context in the client SDK.

[lukas-reining]

Ah yes totally missed that we specified this.
But then the type of the beforeHook is even more confusing.

The current type is:
before?(hookContext: BeforeHookContext, hookHints?: HookHints): Promise<EvaluationContext | void> | EvaluationContext | void

This is correct for the server SDK, for the client I would say it should be just void then:
before?(hookContext: BeforeHookContext, hookHints?: HookHints): void

What do you think @beeme1mr @toddbaert? I could change this then.

[beeme1mr]

Yes, that looks how I would expect.

[lukas-reining]

@toddbaert I think this should also be under the condition that we are using dynamic context:
https://openfeature.dev/specification/sections/hooks#requirement-414

[toddbaert]

@toddbaert I think this should also be under the condition that we are using dynamic context: https://openfeature.dev/specification/sections/hooks#requirement-414

Yes, I think you're right about that.

[toddbaert]

@lukas-reining opened: open-feature/spec#217