How should hooks share data?

[hossam-nasr]

Right now, we have the hookData property in all hook contexts, which allows different hooks of the same cope to share data and communicate with each other. There is also the appHookData, which allows communication across scopes. The behavior of the hookData property is currently inconsistent in terms of allowing overriding it with a new object. Currently, we allow overwriting the hookData property within the same hook type, but not across hook types. We should have consistent behavior between hookData and appHookData, and also consistent behavior for hookData for different hook types/scopes. This issue is to discuss the different options for this behavior. Specifically, the question is should we allow hooks to overwrite hookData with a new object?

Option 1 : No Overwriting

// pre invoc hook 1
preInvocContext.hookData.hello = 'world';

// pre invoc hook 2
preInvocContext.hookData = { foo: 'bar' };

// post invoc hook 1
console.log(postInvocContext.hookData);

This would print:

{
    hello: 'world'
}

Effectively ignoring the changes in pre invoc hook 2, perhaps with also a warning along the lines of "WARNING: Ignoring changes to hook data. You may modify the hook data object, but cannot replace it."

Option 2: Allow Overwriting

// pre invoc hook 1
preInvocContext.hookData.hello = 'world';

// pre invoc hook 2
preInvocContext.hookData = { foo: 'bar' };

// post invoc hook 1
console.log(postInvocContext.hookData);

This would print:

{
    foo: 'bar'
}

Effectively erasing the state set by pre invoc hook 1, perhaps also with a warning message.

Option 3: Object.assign() behaviour:

// pre invoc hook 1
preInvocContext.hookData.hello = 'world';

// pre invoc hook 2
preInvocContext.hookData = { foo: 'bar' };

// post invoc hook 1
console.log(postInvocContext.hookData);

This would print:

{
    hello: 'world',
    foo: 'bar'
}

[ejizba]

Option 4: readonly behaviour:

We can mark hookData readonly (we might have to refactor a tiny bit under the covers to make this work) to make it more clear that you can't overwrite it.

// pre invoc hook 1
preInvocContext.hookData.hello = 'world';

// pre invoc hook 2
preInvocContext.hookData = { foo: 'bar' };

// post invoc hook 1
console.log(postInvocContext.hookData);

This would throw an error during pre invoc hook 2 like hookData is readonly, you cannot set this value and never print anything

Option 5: nice helper methods

Rather than have hookData be a simple object, we can provide helper methods to set and get values. This can help with accidental key overwrites and also make it more clear how to get/set values in general. However I would prefer to put this in the programming model package because it feels more opinionated and I want the core package to be simple/unopinionated.

[ejizba]

My personal preference is 4 and that's the one Alexey also wanted during our discussion offline. @hossam-nasr I didn't think of it during our conversation in the PR otherwise I would've mentioned it earlier. I think the main benefit of this is that it prevents accidental overwriting in a much more obvious way - TypeScript users should get a build error and JavaScript users (or TypeScript users who ignore the build error) will get a runtime error. At that point, I assume users will know to switch to Object.assign if they want to set a bunch of properties at once

[hossam-nasr]

@ejizba I agree option 4 is the least ambiguous and most straightforward of the options. I'm happy to go with that option but would still rather keep it as a separate PR