module: prefer async/await in https imports

[benjamingr]

cc @bmeck @nodejs/modules

This PR refactors the fetch_module fetchWithRedirects function to use the new modern facilities for streams:

• compose instead of pipe for simplified and "more easily correct" error handling on the body stream cc @ronag .
• async/await over new Promise.
• toArray() on the response body instead of manually.

Note this can probably be further simplified by always caching the promise (and never the value) and by moving the "memoize" functionality to a helper).

Fixes: #41950

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[GeoffreyBooth]

cc @JakobJingleheimer

[GeoffreyBooth]

Some minor style notes, I haven’t reviewed in depth.

[benjamingr]

@GeoffreyBooth fixed + added http 404 handling

[bmeck]

Note this can probably be further simplified by always caching the promise (and never the value) and by moving the "memoize" functionality to a helper).

This is actually a bit complicated since import.meta.url needs the value synchronously for the response location after all redirects so it actually relies on storing the value for now.

[benjamingr]

This is actually a bit complicated since import.meta.url needs the value synchronously for the response location after all redirects so it actually relies on storing the value for now.

Ah, mind if I add a comment to the code explaining that around the cacheForGET code?

[bmeck]

Ah, mind if I add a comment to the code explaining that around the cacheForGET code?

I'm all for comments

[benjamingr]

Added both comments.

[JakobJingleheimer]

Oow, fancy. Much cleaner than .on(…) 🙌

[JakobJingleheimer]

Suggested change

	*Itcacheseitherthepromiseorthecahceentrysinceimport.meta.urlneeds
	*Itcacheseitherthepromiseorthecacheentrysinceimport.meta.urlneeds

[benjamingr]

@bmeck @nodejs/modules this could use some reviews :)

[guybedford]

Can we get a 404 test though to land?

[benjamingr]

Added test :)

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/41950 ✔ Done loading data for nodejs/node/pull/41950 ----------------------------------- PR info ------------------------------------ Title module: prefer async/await in https imports (#41950) Author Benjamin Gruenbaum (@benjamingr) Branch benjamingr:use-async-await-in-fetch-module -> nodejs:master Labels esm, needs-ci Commits 1 - module: prefer async/await in https imports Committers 1 - Benjamin Gruenbaum PR-URL: https://github.com/nodejs/node/pull/41950 Fixes: https://github.com/nodejs/node/issues/41950 Reviewed-By: Guy Bedford Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/41950 Fixes: https://github.com/nodejs/node/issues/41950 Reviewed-By: Guy Bedford Reviewed-By: James M Snell -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - module: prefer async/await in https imports ℹ This PR was created on Sat, 12 Feb 2022 18:50:10 GMT ✔ Approvals: 2 ✔ - Guy Bedford (@guybedford): https://github.com/nodejs/node/pull/41950#pullrequestreview-884778590 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41950#pullrequestreview-889949951 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-02-23T18:10:07Z: https://ci.nodejs.org/job/node-test-pull-request/42748/ - Querying data for job/node-test-pull-request/42748/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

[benjamingr]

Landed in 7efef74 🎉

[juanarbol]

This is failing in the v16.x branch; let me work in a backport for this:

/node--inspect--experimental-network-imports--dns-result-order=ipv4firsttest/es-module/test-http-imports.mjs Debugger listeningonws://127.0.0.1:9229/924045ba-ef66-4869-84bf-085849cbf26b Forhelp,see: https://nodejs.org/en/docs/inspector  node:internal/errors:465ErrorCaptureStackTrace(err);^TypeError[ERR_INVALID_ARG_TYPE]: The"list"argumentmustbeaninstanceofArray.ReceivedaninstanceofBufferatconcat(node:buffer:537:3) at node:internal/modules/esm/fetch_module:165:24atprocessTicksAndRejections(node:internal/process/task_queues:96:5)atasync node:internal/modules/esm/fetch_module:171:7atasyncdefaultLoad(node:internal/modules/esm/load:21:14)atasyncESMLoader.load(node:internal/modules/esm/loader:407:20)atasyncESMLoader.moduleProvider(node:internal/modules/esm/loader:326:11)atasynclink(node:internal/modules/esm/module_job:70:21){ code: 'ERR_INVALID_ARG_TYPE'}

Looks like the toArray() method is returning the Buffer not the array, see: https://github.com/nodejs/node/pull/41553/files#diff-00d8d040a3fe90e3052af39cb55dc9bcac70539d5d17791449cbfe7e3b55fb90R1903-R1904