Skip to content

Document context merging order for before hook ctx#125

Merged
beeme1mr merged 3 commits into
mainfrom
context-merging-order
Aug 9, 2022
Merged

Document context merging order for before hook ctx#125
beeme1mr merged 3 commits into
mainfrom
context-merging-order

Conversation

@justinabrahms

Copy link
Copy Markdown
Member

Fixes #124

Signed-off-by: Justin Abrahms <jabrahms@ebay.com>
Comment thread specification/sections/04-hooks.md Outdated
@justinabrahms justinabrahms force-pushed the context-merging-order branch from 2f4e4c9 to 5527a48 Compare August 8, 2022 17:08
Co-authored-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: Justin Abrahms <jabrahms@ebay.com>
@justinabrahms justinabrahms force-pushed the context-merging-order branch from 5527a48 to f2ce332 Compare August 8, 2022 17:09
@toddbaert toddbaert self-requested a review August 8, 2022 17:09

@toddbaert toddbaert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I dont love that this is a bit redundant with 3.2.2, but that was true before this change and I'm not sure the best way to resolve it. I think this is an improvement.

@beeme1mr beeme1mr self-requested a review August 8, 2022 17:30
@toddbaert toddbaert self-requested a review August 8, 2022 18:19
@toddbaert

toddbaert commented Aug 8, 2022

Copy link
Copy Markdown
Member

I think this made perfect sense before provider hooks, but now with them in the mix, things get more complicated.

One of the primary use-cases of provider hooks is context transformation... if a provider hook does significant context transformation (say, into some vendor-specific object, or by serializing values in some required way) then it has to have the highest merge priority, right?

So do we do:

(top-most wins):

  1. before hook
  2. invocation
  3. client
  4. api

or

(top-most wins):

  1. provider before hooks
  2. invocation
  3. before hook
  4. client
  5. api

@justinabrahms

Copy link
Copy Markdown
Member Author

Huh. I don't like our options now. :-P

You're thinking that a context transformation use-case is going to set an additional property on the context that's specific for the provider?

How about this (top-most wins)?

  1. provider before hooks
  2. invocation hooks
  3. invocation context
  4. client before hook
  5. client context
  6. api before hook
  7. api context

@toddbaert

Copy link
Copy Markdown
Member

You're thinking that a context transformation use-case is going to set an additional property on the context that's specific for the provider?

Well yes, but to be honest, I was thinking something even more potentially problematic - our context transformer allowed the context object to be an entirely new object/instance - for example an LDUser. I'm not sure that was understood 😅 . As you can imagine merging after that transformation is completely impossible.

At the very least, I think provider hooks may transform the context in a way to maintain compatibility with their particular backend, so they need to "win".

@toddbaert

toddbaert commented Aug 8, 2022

Copy link
Copy Markdown
Member

Thinking about it more, considering implementation difficulty and cognitive burden on application-authors, I think I'm in favor of:

(top-most wins):

  1. before hooks (all of them, including provider hooks)
  2. invocation
  3. client
  4. api

So hooks will always win... I think this is OK and simple to understand. If the author REALLY wants to override something in their invocation, they can use an invocation before hook to do that, which will override everything except the provider hooks (which is ideal).

@justinabrahms @beeme1mr what do you guys think about that?

@justinabrahms

Copy link
Copy Markdown
Member Author

I think I could be into that. So we're saying that:

  • We don't really expect you to specify content in your invocation context that will collide w/ things the hooks provide.
  • If you do have a conflict you want to exclude, we expect you to define an inline hook.
  • You cannot override before hook values from a provider.

@toddbaert

Copy link
Copy Markdown
Member

I think I could be into that. So we're saying that:

  • We don't really expect you to specify content in your invocation context that will collide w/ things the hooks provide.
  • If you do have a conflict you want to exclude, we expect you to define an inline hook.
  • You cannot override before hook values from a provider.

Exactly.

Signed-off-by: Justin Abrahms <jabrahms@ebay.com>
@beeme1mr beeme1mr merged commit c5b7b28 into main Aug 9, 2022
@beeme1mr beeme1mr deleted the context-merging-order branch August 9, 2022 01:29
justinabrahms pushed a commit that referenced this pull request Aug 9, 2022
Signed-off-by: Justin Abrahms <jabrahms@ebay.com>
Co-authored-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: Justin Abrahms <jabrahms@ebay.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hook context merging w/ api ctx

4 participants