-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
discuss: remove domain module #45824
Comments
a good idea would be to test how this will effect citgm |
this is the previous discussion opened more than 5 years ago: #10843
It seems that it is something that has been around for quite some time now, I think is time to remove |
They've been deprecated even longer, the deprecation notice was already there in the first io.js release! (ref. 6a29356) I'll open a trial PR and see how it fares. |
https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3060/ - honestly, there are so many random test failures (also in other, unrelated citgm runs) that it's hard to say whether it's indicative of anything. FWIW, I didn't find any mentions of the domain module while randomly clicking through some of the failing test suites. |
Ah, thanks. Okay, so that means a runtime deprecation first before the hammer comes down. |
I for one, vote to keep Domains - I use them for multiple use cases - is there a clear replacement for the functionality? Until there is a clear replacement, I vote to keep in core. Make something better, before just removing it. Test libraries like In other words: The domain API is simple to use, intuitive, and works, however it is a bit slow in performance. I would like AsyncLocalStorage (or whatever replaces Domain) to be beyond "experimental" before removing Domains when there is no replacement for the Domain functionality (which is critical for some people who use in production). For example middleware similar to this can improve production servers: app.use((req,res,next) => {
const d = Domain.create();
d.once('error', e => {
res.status(500).send(e.message);
});
d.run(next);
}); if there is a new api that can what the above does, and improve performance, I am all for it. 👍 For example: |
Keep Domain. The AsyncLocalStorage and async_hooks just aren't reliable. |
FWIW domain uses async hooks internally so I doubt it's more reliable. |
require('domain')
was deprecated 7.5 years agomodern code uses
AbortController
bug tracker activity suggests it's not in active use (or it works so well people never hit bugs0 - unlikely!)
the supporting infrastructure for
domain
is a kind of technical debt that also has performance implications1; it's not as bad as it used to be but it's still thereProposal:
remove it for good in the next major release
leave a stub
lib/domain.js
that prints a warning, a la thesys
module.0 the prime reason for deprecation was because it was so buggy due to part poor design, part poor implementation
1 at one time it was so bad that if even one module imported
domain
, whole-program performance tankedThe text was updated successfully, but these errors were encountered: