Skip to content
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

Jump to bottom

Possible fix for #15374 #15389

Closed
wants to merge 1 commit into from
Closed

Possible fix for #15374 #15389

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion lib/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ Module._load = function(request, parent, isMain) {
if (experimentalModules) {
if (filename === null || /\.mjs$/.test(filename)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why regex is used here instead of filename.endsWith('.mjs')?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bmeck?

Copy link
Member

Choose a reason for hiding this comment

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

Habits from before that existed. Technically we could/should not be using prototype functions since they could be tainted by preloading via --require.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack.

Copy link
Member

@jdalton jdalton Sep 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Technically we could/should not be using prototype functions since they could be tainted by preloading

There's a lot to tweak then because there's heavy use of apply, call, charAt, concat, pop, push, slice, test, and on and on. It'd be neat to see if some of this could be solved with a new context to avoid plucking all these methods off.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot to tweak then because there's heavy use of charAt, concat, pop, push, test, slice, and on and on.

This concern pops up now and then.
Refs: #12956
Refs: #12981

Copy link
Member

Choose a reason for hiding this comment

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

@jdalton Contexts are expensive w/o snapshots to create and we would need a frozen one to avoid mutations which would mean making that custom Snapshot, we couldn't use vm.createContext due to the funky sandbox bridge it creates leaking : #6283 . I'd be open to using one if we had a snapshot, but it would still be ~10ms on my benchmarks to create a full one.

try {
ESMLoader.import(request).catch((e) => {
ESMLoader.import(getURLFromFilePath(filename).href).catch((e) => {
console.error(e);
process.exit(1);
});
Expand Down