Skip to content

lib: refactor lazy loading of undici for fetch method #52275

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.

Sign up for GitHub

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

Merged
nodejs-github-bot merged 3 commits into from
Apr 12, 2024
Merged

lib: refactor lazy loading of undici for fetch method #52275

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3fdd4d3
lib: refactor lazy loading of undici for fetch method
YCChenVictor Mar 18, 2024
5c6a889
lib: implement lazy loading for fetch function
YCChenVictor Apr 7, 2024
7937b69
lib: test for fetch mock without prior invocation
YCChenVictor Apr 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
lib: implement lazy loading for fetch function 
Instead of importing undici upfront, the module is now conditionally
required using require only when the fetch function is called for the
first time and the undici implementation is not already available. This
lazy loading approach improves resource usage and test reliability by
loading undici only when needed.
  • Loading branch information
@YCChenVictor
YCChenVictor committed Apr 9, 2024
commit 5c6a8893db5541d53cb9c05fbc2324fb9ec1e5ca
12 changes: 7 additions & 5 deletions lib/internal/bootstrap/web/exposed-window-or-worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,19 @@ defineReplaceableLazyAttribute(globalThis, 'perf_hooks', ['performance']);
const { installObjectURLMethods } = require('internal/url');
installObjectURLMethods();

let fetchImpl;
// https://fetch.spec.whatwg.org/#fetch-method
ObjectDefineProperty(globalThis, 'fetch', {
__proto__: null,
configurable: true,
enumerable: true,
writable: true,
value: function fetch(input, init = undefined) { // eslint-disable-line func-name-matching
// Loading undici alone lead to promises which breaks lots of tests so we
// have to load it really lazily for now.
const { fetch: impl } = require('internal/deps/undici/undici');
return impl(input, init);
value: function value(input, init = undefined) {
Copy link
Copy Markdown

@hulkish hulkish Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't init = undefined redundant here?

Copy link
Copy Markdown
Contributor

@regseb regseb Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's to specify that the init argument is optional.

Copy link
Copy Markdown

@hulkish hulkish Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but you're defaulting it to undefined -.which would be the case if it wasn't defined, it's redundant

Copy link
Copy Markdown

@hulkish hulkish Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment for https://github.com/nodejs/node/blob/main/deps/undici/undici.js#L12497

It makes no sense to default an argument value to undefined ever

Copy link
Copy Markdown
Contributor

@bakkot bakkot Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It affects the .length of the function, which is observable. Parameters with default values don't count towards the length. And since the correct length is 1, not 2, this is the only way to do that without either explicitly setting the length attribute or referencing arguments.

if (!fetchImpl) { // Implement lazy loading of undici module for fetch function
const undiciModule = require('internal/deps/undici/undici');
fetchImpl = undiciModule.fetch;
}
return fetchImpl(input, init);
},
});

Expand Down