lib: refactor lazy loading of undici for fetch method by YCChenVictor · Pull Request #52275 · nodejs/node

YCChenVictor · 2024-03-30T14:26:41Z

Object.defineProperty is updated to lazily load the undici dependency for the fetch method. This change allows for simpler and more reliable mocking of the fetch method for testing purposes, resolving issues encountered with premature method invocation during testing.

Fixes: #52015

nodejs-github-bot · 2024-03-30T14:26:46Z

Review requested:

@nodejs/startup
@nodejs/web-standards

mcollina

Note that this setup slows down every invocation of fetch() by adding an additional function call and the require logic. The previous logic added lazy loading that should be kept in.

Could you add a test? I don't understand what would not be possible in the previous setup.

MoLow · 2024-03-31T07:43:28Z

Could you add a test? I don't understand what would not be possible in the previous setup.

see #52015
mocking globalThis.fetch is harder without this fix.

MoLow · 2024-03-31T07:43:38Z

Could you add a test? I don't understand what would not be possible in the previous setup.

see #52015
mocking globalThis.fetch is harder without this fix.

mcollina · 2024-03-31T13:09:29Z

The test should be added and the lazyloading kept in.

regseb · 2024-04-04T14:04:48Z

@mcollina

I think this configuration is faster on the first call (there's no need to replace get and set by value). And the speed is the same for next calls.

console.log("BEFORE");console.log(Object.getOwnPropertyDescriptor(globalThis,"fetch"));console.log(Object.getOwnPropertyDescriptor(globalThis,"fetch").set.toString());console.log(Object.getOwnPropertyDescriptor(globalThis,"fetch").get.toString());fetch;console.log("AFTER");console.log(Object.getOwnPropertyDescriptor(globalThis,"fetch"));console.log(Object.getOwnPropertyDescriptor(globalThis,"fetch").value.toString());

When I run the above code in Node.js v21.7.0:

BEFORE
{
get: [Function: get],
set: [Function: set],
enumerable: true,
configurable: true
}
function set(value) {
ObjectDefineProperty(globalThis, 'fetch', {
__proto__: null,
writable: true,
value,
});
}
get() {
function fetch(input, init = undefined) {
// 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);
}
set(fetch);
return fetch;
}
AFTER
{
value: [Function: fetch],
writable: true,
enumerable: true,
configurable: true
}
function fetch(input, init = undefined) {
// 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);
}

With this pull request, the AFTER configuration is directly available.

Lazyloading is kept, as undici is only imported when fetch() is run.

atlowChemi · 2024-04-05T13:01:50Z

do we really need to lazily load it every time?
Can't we have something around the lines of:
letimpl;value: functionfetch(input,init=undefined){impl??=require('internal/deps/undici/undici');returnimpl(input,init)},

Technically require would cache it anyway, right?

Well, technically it will, but the cost for calling require would still be higher (I don't have benchmarks, but it has more overhead than holding a local variable in memory)

mcollina · 2024-04-05T13:16:16Z

What @atlowChemi wrote in #52275 (comment)

regseb · 2024-04-04T14:07:36Z

I think the curly bracket is no longer necessary. It was probably used to avoid creating the set function in the root scope.

Object.defineProperty is updated to lazily load the undici dependency for the fetch method. This change allows for simpler and more reliable mocking of the fetch method for testing purposes, resolving issues encountered with premature method invocation during testing. Fixes: nodejs#52015

atlowChemi

Other than lint errors, LGTM

mcollina

lgtm

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.

MoLow · 2024-04-09T07:52:40Z

@YCChenVictor can you add a test that checks #52015 won't regress?

Ethan-Arrowood

how does this work with the following lines (just below your contribution?):

exposeLazyInterfaces(globalThis,'internal/deps/undici/undici',['FormData','Headers','Request','Response',]);

If I use any of those interfaces, should fetch be loaded as well?

nodejs-github-bot · 2024-04-11T08:21:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58262/

nodejs-github-bot · 2024-04-11T10:13:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58271/

YCChenVictor · 2024-04-12T03:27:30Z

I'm uncertain why the checks failed. Perhaps I should rebase this branch on the latest node commit? Any guidance would be greatly appreciated, and I'm sincerely grateful for any corrections or suggestions. Thank you!

nodejs-github-bot · 2024-04-12T05:36:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58305/

nodejs-github-bot · 2024-04-12T08:49:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58307/

nodejs-github-bot · 2024-04-12T11:29:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58311/

nodejs-github-bot · 2024-04-12T13:15:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58318/

joyeecheung · 2024-04-12T13:34:18Z

The failures are most likely flakes.

nodejs-github-bot · 2024-04-12T14:12:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58323/

nodejs-github-bot · 2024-04-12T17:46:27Z

Commit Queue failed

- Loading data for nodejs/node/pull/52275
✔ Done loading data for nodejs/node/pull/52275
----------------------------------- PR info ------------------------------------
Title lib: refactor lazy loading of undici for fetch method (#52275)
Author Victor Chen (@YCChenVictor, first-time contributor)
Branch YCChenVictor:feature/lazy-fetch-undici-loading -> nodejs:main
Labels lib / src, needs-ci, commit-queue-squash
Commits 3
- lib: refactor lazy loading of undici for fetch method
- lib: implement lazy loading for fetch function
- lib: test for fetch mock without prior invocation
Committers 1
- YCChen PR-URL: https://github.com/nodejs/node/pull/52275
Fixes: https://github.com/nodejs/node/issues/52015
Reviewed-By: Moshe Atlow Reviewed-By: Matteo Collina Reviewed-By: Chemi Atlow Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52275
Fixes: https://github.com/nodejs/node/issues/52015
Reviewed-By: Moshe Atlow Reviewed-By: Matteo Collina Reviewed-By: Chemi Atlow Reviewed-By: James M Snell --------------------------------------------------------------------------------
⚠ Commits were pushed since the last approving review:
⚠ - lib: test for fetch mock without prior invocation
ℹ This PR was created on Sat, 30 Mar 2024 14:26:41 GMT
✔ Approvals: 4
✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/52275#pullrequestreview-1990083304
✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52275#pullrequestreview-1985970496
✔ - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/52275#pullrequestreview-1988005649
✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/52275#pullrequestreview-1992479891
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2024-04-12T14:12:32Z: https://ci.nodejs.org/job/node-test-pull-request/58323/
- Querying data for job/node-test-pull-request/58323/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/8665947485

nodejs-github-bot · 2024-04-12T17:54:44Z

Landed in 2cd3073

Object.defineProperty is updated to lazily load the undici dependency for the fetch method. This change allows for simpler and more reliable mocking of the fetch method for testing purposes, resolving issues encountered with premature method invocation during testing. Fixes: #52015 PR-URL: #52275 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 30, 2024

MoLow approved these changes Mar 31, 2024
View reviewed changes

mcollina requested changes Mar 31, 2024
View reviewed changes

fatso83 mentioned this pull request Apr 4, 2024
[Node v21.7.0] Impossible to stub fetchsinonjs/sinon#2590
Closed

atlowChemi reviewed Apr 5, 2024
View reviewed changes

regseb reviewed Apr 5, 2024
View reviewed changes

YCChenVictor force-pushed the feature/lazy-fetch-undici-loading branch from 796a413 to 9ee922eCompare April 7, 2024 05:58

MoLow approved these changes Apr 7, 2024
View reviewed changes

MoLow requested a review from mcollina April 7, 2024 07:33

atlowChemi approved these changes Apr 7, 2024
View reviewed changes

Comment threadlib/internal/bootstrap/web/exposed-window-or-worker.js Outdated

mcollina approved these changes Apr 8, 2024
View reviewed changes

YCChenVictor force-pushed the feature/lazy-fetch-undici-loading branch from 9ee922e to 5c6a889Compare April 9, 2024 01:14

atlowChemi approved these changes Apr 9, 2024
View reviewed changes

MoLow added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 9, 2024

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2024

panva added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 9, 2024

YCChenVictor force-pushed the feature/lazy-fetch-undici-loading branch from b42ee5b to 2eddedaCompare April 9, 2024 13:25

Ethan-Arrowood reviewed Apr 9, 2024
View reviewed changes

MoLow approved these changes Apr 9, 2024
View reviewed changes

YCChenVictor force-pushed the feature/lazy-fetch-undici-loading branch from 2eddeda to 7937b69Compare April 11, 2024 08:08

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 11, 2024

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 11, 2024

joyeecheung mentioned this pull request Apr 12, 2024
Node v20.12.x TypeError: fetch is not a function #52481
Closed

joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 12, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 12, 2024

joyeecheung approved these changes Apr 12, 2024
View reviewed changes

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 12, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 12, 2024

nodejs-github-bot merged commit 2cd3073 into nodejs:mainApr 12, 2024

yeoffrey mentioned this pull request Apr 26, 2024
breaking change in 21.7.0 when mocking fetch #52015
Closed

aduh95 mentioned this pull request Apr 30, 2024
2024-05-02, Version 22.1.0 (Current) #52768
Merged

marco-ippolito mentioned this pull request May 2, 2024
v20.13.0 proposal #52793
Merged

dygabo mentioned this pull request May 31, 2024
lib: fix the name of the fetch global function #53227
Merged

Uh oh!

Uh oh!

Conversation

YCChenVictor commented Mar 30, 2024

Uh oh!

nodejs-github-bot commented Mar 30, 2024

Uh oh!

mcollina left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MoLow commented Mar 31, 2024

Uh oh!

MoLow commented Mar 31, 2024

Uh oh!

mcollina commented Mar 31, 2024

Uh oh!

regseb commented Apr 4, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atlowChemiApr 5, 2024

Choose a reason for hiding this comment

Uh oh!

hulkishApr 5, 2024

Choose a reason for hiding this comment

Uh oh!

atlowChemiApr 6, 2024

Choose a reason for hiding this comment

Uh oh!

mcollina commented Apr 5, 2024

Uh oh!

regsebApr 4, 2024

Choose a reason for hiding this comment

Uh oh!

atlowChemi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

MoLow commented Apr 9, 2024

Uh oh!

Ethan-Arrowood left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 11, 2024

Uh oh!

nodejs-github-bot commented Apr 11, 2024

Uh oh!

YCChenVictor commented Apr 12, 2024

Uh oh!

nodejs-github-bot commented Apr 12, 2024

Uh oh!

nodejs-github-bot commented Apr 12, 2024

Uh oh!

nodejs-github-bot commented Apr 12, 2024

Uh oh!

nodejs-github-bot commented Apr 12, 2024

Uh oh!

joyeecheung commented Apr 12, 2024

Uh oh!

nodejs-github-bot commented Apr 12, 2024

Uh oh!

nodejs-github-bot commented Apr 12, 2024

Uh oh!

nodejs-github-bot commented Apr 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

mcollina left a comment •
edited
Loading

regseb commented Apr 4, 2024•
edited
Loading