-
-
Notifications
You must be signed in to change notification settings - Fork 664
fix: avoid setup global dispatcher in advance #4357
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
base: main
Are you sure you want to change the base?
Conversation
18a7803
to
db91409
Compare
CI doesn't seem happy |
Yeah I see, trying to figure out |
2f72fee
to
ac9a14b
Compare
ac9a14b
to
84533e6
Compare
@mcollina updated the code |
}, | ||
set (agent) { | ||
setGlobalDispatcher(agent) | ||
}, |
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.
Unfortunately this implementation is not equivalent. After this change, whoever is reading the dispatcher would invoke a function, which would generate a slight overhead. The solution is on first access to remove all this getter business.
throw new InvalidArgumentError('Argument agent must implement Agent') | ||
} | ||
currentDispatcher = agent | ||
} |
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'm relatively certain this function is already available somewhere
Would it make more sense for undici to always overwrite the global dispatcher (if it's not from undici already)? Importing |
This relates to...
Rationale
Changes
lazy setup dispatcher so that lazyLoadUndici in node.js core module won't affect the
globalThis
Features
Bug Fixes
Fixes: nodejs/node#59012
Breaking Changes and Deprecations
Status