Skip to content

Comments

fix(core): Allow to return a new hook context in basic hooks#2462

Merged
daffl merged 2 commits intodovefrom
replace-hook-context-2451
Oct 5, 2021
Merged

fix(core): Allow to return a new hook context in basic hooks#2462
daffl merged 2 commits intodovefrom
replace-hook-context-2451

Conversation

@daffl
Copy link
Member

@daffl daffl commented Oct 5, 2021

This fixes a regression in backwards compatibility that is difficult to debug.

Closes #2451

@daffl daffl changed the title fix(core): Allow to return aa new hook context in basic hooks fix(core): Allow to return a new hook context in basic hooks Oct 5, 2021
@daffl daffl merged commit 422b6fc into dove Oct 5, 2021
@daffl daffl deleted the replace-hook-context-2451 branch October 5, 2021 23:27
@vonagam
Copy link
Member

vonagam commented Oct 13, 2021

Wait, but it wasn't an unexpected regression, it was a deliberate choice for depreciation in next major version advocated by me in #1443.

This current fix, while small and easy creates a different behavior from v4. Here you just Object.assign returned object, before discarding it, to original context. Examples of problems that may lead to:

  • If somebody to delete property (custom or error, id) from a new context, that change won't be transferred by Object.assign and will be lost.
  • Map/WeakMap with context as a key will not work, since while one will think that they have a reference to new current context, what they actually have is an update to original one.
  • As before, if you start making changes to new context object but then something throws an error, all your changes are lost, which creates difference in behavior with non-returning hooks.
  • There is also a composition problem in v4 that i did not mention before - if you work with arbitrary passed-in hooks then you yourself should handle cases when they return contexts and the only consistent working way is to start returning too, which leads to propagation of a bad practice.

And migration is easy, they could have done the same that was done here - instead of return {...context, ...update} just write Object.assign(context, update) - it works (and more proper) both in v4 and v5.

Anyway, you can ignore this message, just had to write since it was me who proposed depreciation.

@daffl
Copy link
Member Author

daffl commented Oct 15, 2021

Ah yes, sorry I remember now. The problem with this approach was that I heard from a couple of beta testers now that they are ensuring immutability in their projects and did something like:

context => {
  return {
    ...context,
    params
  }
}

The problem with the new implementation was that it just did nothing and you had no idea why. You may be right in that it makes more sense to throw an error if a different context than the original one is returned.

@vonagam
Copy link
Member

vonagam commented Oct 15, 2021

had no idea why

Legacy hook return type should have been updated to reflect that returning is not an option anymore. Then they could see (at least those who use typescript) an error at writing time instead of compile/runtime one.

ensuring immutability

Good intention, but stems from miscommunication (docs and types). Originally returning new context was added (as you said) with an idea of a pure hook (receive input, return output without side effects), but it was never actually implemented like that in the end. So no profits, only costs for that mirage...

Another smallish thing is that migration from void hooks to async hooks are easier than from returning ones. So if you plan to force migration to new hooks in the distant future (one or two versions down the line), they would still need to update.

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.

returned hook context not used anymore in workflow

2 participants