New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: replace CLS with ALS, enable ALS by default, discourage unmanaged transactions #15292
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Don't forget to remove cls-hooked
from package.json, apart from that it's just a few typos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a last reference to CLS here;
describe('_logQuery', () => { |
But we should be good after that
I don't know why that's called CLS but it doesn't seem to be related to Continuation local storage |
Maybe rename it to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do the rename in a different PR
Yes let's leave it as is, we'll update the test suite when we migrate it and/or query.js to TypeScript |
The |
It is, the update is in sequelize/website#419, which I need to update & get merged |
Pull Request Checklist
Description Of Change
Closes #15291
Closes #13742
This PR drops CLS in favor of using node's built-in AsyncLocalStorage.
The AsyncLocalStorage is now per Sequelize instance instead of global. It never made sense for this option to be global.
It also enables ALS transactions by default. They can be disabled by setting
disableAlsTransactions
to true in Sequelize's options.Finally, in order to discourage using unmanaged transactions (which are easy to misuse), this PR removes the ability to get an unmanaged transaction by not providing a callback to
sequelize.transaction()
.Unmanaged transactions are now started using
sequelize.startUnmanagedTransaction()
instead.Todos