Add AsyncLocalStorage driver #39
Conversation
After doing research on what `bindEmitter` does, I could not see the benefits of using it. At the time the hooks run, the transaction is already over. There seem to be no reason to preserve store context for userland use. All current tests pass. There is no mention of preserved context in the README, therefore this should be treated as internal change. This change is motivated by the fact AsyncLocalStorage does not support `bindEmitter` out of the box and without understanding the end goal it is not feasible to properly reimplement it. If at any point in the future a viable use-case is provided, the bindEmitter can easily be restored.
This change introduces a concept of StorageLayer, an interface which can be implemented using various storage providers. Currently the implementations include AsyncLocalStorage and cls-hooked.
This commit abstracts the use of cls-hooked to StorageLayer. It also updates the StorageLayer provider to always use cls-hooked implementation, as AsyncLocalStorage implementation does not pass tests yet.
The change implements AsyncLocalStorage driver, which passes all existing tests. As the actual logic relies on cls-hooked behavior a Store class is introduces to emulate it within AsyncLocalStorage.
Separating name refactor out of previous commit for readability
|
Hi, it looks cool. Let me check it on these weekends. |
|
Happy to test and start using it, once merged! |
|
Hi guys, sorry for such delay. It's been a busy period at work. I'll get it done today or tomorrow morning. |
|
All tests have successfully passed. However, as rightly mentioned above, the current code does not allow for convenient testing of this feature without running it with different We need to modify the internal data structure, but this would be beyond the scope of the current PR. I believe we can roll out this feature and then I can focus on the necessary internal modifications. Additionally, I also think we need to use |
|
For tests, alternatively, you could run CI with different environments using matrix feature. As for AUTO, I am not sure what is the best rollout strategy here - to release minor keeping cls hooked the default and change to AUTO with next major. Or release major right away. |
|
Let me know if I can help with it |
I will check, thanks.
Hard question 🙃. Actually, I'm not thinking about a major version yet. In my opinion it would be better to release the minor version first with cls hooked the default as it is now. Next major, as you said, we can change to AUTO. |
|
Thank you for your patience guys. I've merged this PR. |
So I decided to spent the day implementing AsyncLocalStorage driver... Resolves #22
The PR is best read commit after commit, as I tried to make incremental changes:
storageDriveroption, defaults to cls-hookedNeed to figure out a way to test both drivers on the same suite. It seems infeasible to copy-and-paste cases, or worse prepare some specific cases for drivers. Best to run everything twice to ensure 100% combability. The lib sadly stores some information in memory (eg.
dataSourcesmap) and there is currently no way to clean it up between runs, so.eachfails. Therefore I temporarily addedTEST_STORAGE_DRIVERenvironment variable that can be defined before tests run:TEST_STORAGE_DRIVER=ASYNC_LOCAL_STORAGE npm run testAs I use newer version of
npm, the package-lock.json got updated. There is one new dependency,semver. It could probably be removed if we decided on custom implementation, but I worried about the compatibility with older runtimes which may use unexpected version format.Considerations:
storageDriveroption to be set toAUTO. For now it is set toCLS_HOOKEDavoiding breaking change. By default package works as before, the new driver is opt-in.bindEmitterwas removed; no tests indicated it's usefulness. Reading the implementation I haven't found any sensible scenario in which it would be required as part of this package. It is possible to recreate (not very hard) if needed, please provide test case.reset()method could be provided - even if only for tests. Did not want to make too big of a change.