-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
module: prefer async/await in https imports #41950
module: prefer async/await in https imports #41950
Conversation
Review requested:
|
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.
Some minor style notes, I haven’t reviewed in depth.
@GeoffreyBooth fixed + added http 404 handling |
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? |
I'm all for comments |
Added both comments. |
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.
Oow, fancy. Much cleaner than .on(…)
🙌
@@ -32,6 +30,9 @@ const path = require('path'); | |||
/** | |||
* Only for GET requests, other requests would need new Map | |||
* HTTP cache semantics keep diff caches | |||
* | |||
* It caches either the promise or the cahce entry since import.meta.url needs |
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.
* It caches either the promise or the cahce entry since import.meta.url needs | |
* It caches either the promise or the cache entry since import.meta.url needs |
@bmeck @nodejs/modules this could use some reviews :) |
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.
Can we get a 404 test though to land?
67713f1
to
ace0812
Compare
Added test :) |
b3ef5cb
to
b575c6d
Compare
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/.ncuhttps://github.com/nodejs/node/actions/runs/1889467631 |
Landed in 7efef74 🎉 |
PR-URL: nodejs#41950 Fixes: nodejs#41950 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#41950 Fixes: nodejs#41950 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This is failing in the v16.x branch; let me work in a backport for this: /node --inspect --experimental-network-imports --dns-result-order=ipv4first test/es-module/test-http-imports.mjs
Debugger listening on ws://127.0.0.1:9229/924045ba-ef66-4869-84bf-085849cbf26b
For help, see: https://nodejs.org/en/docs/inspector
node:internal/errors:465
ErrorCaptureStackTrace(err);
^
TypeError [ERR_INVALID_ARG_TYPE]: The "list" argument must be an instance of Array. Received an instance of Buffer
at concat (node:buffer:537:3)
at node:internal/modules/esm/fetch_module:165:24
at processTicksAndRejections (node:internal/process/task_queues:96:5)
at async node:internal/modules/esm/fetch_module:171:7
at async defaultLoad (node:internal/modules/esm/load:21:14) at async ESMLoader.load (node:internal/modules/esm/loader:407:20)
at async ESMLoader.moduleProvider (node:internal/modules/esm/loader:326:11)
at async link (node:internal/modules/esm/module_job:70:21) {
code: 'ERR_INVALID_ARG_TYPE'
} Looks like the |
PR-URL: nodejs/node#41950 Fixes: nodejs/node#41950 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
cc @bmeck @nodejs/modules
This PR refactors the fetch_module
fetchWithRedirects
function to use the new modern facilities for streams:compose
instead ofpipe
for simplified and "more easily correct" error handling on the body stream cc @ronag .async/await
overnew 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