-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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
esm: synchronise default path of ModuleLoader.load
#57390
Conversation
Review requested:
|
context = { __proto__: context, source }; | ||
} | ||
|
||
if (format == null) { | ||
// Now that we have the source for the module, run `defaultGetFormat` to detect its format. | ||
format = await defaultGetFormat(urlInstance, context); | ||
format = defaultGetFormat(urlInstance, context); |
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.
defaultGetFormat
is already sync
ModuleLoader
ModuleLoader.load
ModuleLoader.load
ModuleLoader.load
const { readFile: readFileAsync } = require('internal/fs/promises').exports; | ||
source = await readFileAsync(url); | ||
source = readFileSync(url); |
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.
This converts to a blocking op, but I'm not sure if that matters.
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.
The CommonJS loadSource
uses readFileSync
:
node/lib/internal/modules/cjs/loader.js
Line 1128 in fbe37d5
return fs.readFileSync(filename, 'utf8'); |
though CommonJS is fundamentally sync. I think it should be fine to use the same in ESM; @joyeecheung or anyone else, do you have any concerns?
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.
Blocking fs is generally not problematic when your fs is fast enough (and not taking on the async overhead might make it faster, as nodejs/performance#151 shows).
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.
+1 to the effort. Tracking issue: #55782
We should also find some benchmarks that measure the performance impact of the overall syncification effort. Perhaps the existing ESM benchmarks are sufficient?
const { readFile: readFileAsync } = require('internal/fs/promises').exports; | ||
source = await readFileAsync(url); | ||
source = readFileSync(url); |
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.
The CommonJS loadSource
uses readFileSync
:
node/lib/internal/modules/cjs/loader.js
Line 1128 in fbe37d5
return fs.readFileSync(filename, 'utf8'); |
though CommonJS is fundamentally sync. I think it should be fine to use the same in ESM; @joyeecheung or anyone else, do you have any concerns?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57390 +/- ##
==========================================
- Coverage 90.23% 90.11% -0.12%
==========================================
Files 630 630
Lines 185200 185193 -7
Branches 36233 36165 -68
==========================================
- Hits 167108 166882 -226
- Misses 11056 11239 +183
- Partials 7036 7072 +36
🚀 New features to boost your workflow:
|
This PR got created wrong, so I had to create a new one to update it: #57419 Apologies for the noise 😞 |
No description provided.