Closed Aareksio closed 11 months ago
Hi, it looks cool. Let me check it on these weekends.
Happy to test and start using it, once merged!
@Aliheym @Aareksio love this feature,
is this lib abandoned, and it's better to host it on our own to get this feature?
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 TEST_STORAGE_DRIVER
env.
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 AUTO
as the default value
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
For tests, alternatively, you could run CI with different environments using matrix feature.
I will check, thanks.
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.
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 https://github.com/Aliheym/typeorm-transactional/issues/22
The PR is best read commit after commit, as I tried to make incremental changes:
storageDriver
option, 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.
dataSources
map) and there is currently no way to clean it up between runs, so.each
fails. Therefore I temporarily addedTEST_STORAGE_DRIVER
environment variable that can be defined before tests run:As 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:
storageDriver
option to be set toAUTO
. For now it is set toCLS_HOOKED
avoiding breaking change. By default package works as before, the new driver is opt-in.bindEmitter
was 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.