-
Notifications
You must be signed in to change notification settings - Fork 34
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
Hook ordering & state #40
Comments
I think well-defined ordering is certainly a good idea, likely a necessity. I created a stub markdown file for hooks in the spec: I'm less sure about storing arbitrary state on the HookContext. It might be the right approach, but I also am wary of encouraging dependencies between hooks that way. It could be a footgun. In the sample OTel Hook, we used a private WeakMap member (Java has this concept too: https://docs.oracle.com/javase/7/docs/api/java/util/WeakHashMap.html) to store state, using the Hook Context object reference itself as the key (since the reference doesnt change across flag evaluation, the Do you think this pattern would satisfy your use-case @justinabrahms ? |
Without passing state, I'm not clear how we could represent: I want this hook to do the thing it does on every invocation... except this one in particular. The other thing we've talked about is having users put some sentinel values within the evaluation context, which feels super gross. Maybe we have some immutable state that's included on FlagEvaluationOptions when the user kicks off the request? |
Very gross, I couldn't agree more on that. It actually even caused bugs in some vendor SDKs in my JS experimentation, because of circular references that broke JSON parsing (presumably to build an HTTP POST body).
Ya, I think that's certainly valid, I'm just not sure about doing it with "side-effects" between hooks.
I think this is a really good solution, and a perfect use of I should add, I'm not 100% opposed to the mutable state between hooks, I just suspect it's something we should avoid if possible. I could be wrong though 🤷 . |
Addressed by #45. Once that's merged we can likely close this. |
Addressed by hook hints |
We need to have a concept of hook ordering. In our case, we have a client hook
EmitMetrics
. Sometimes, devs don't want to emit metrics, so we want to selectively overwrite it.We're thinking that we'll build this like:
This means we'll need precise control around when hooks run (with associated APIs to insert at specific ordering) as well as a change to HookContext to hold some state.
The text was updated successfully, but these errors were encountered: