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.

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: support Wasm without file extension within module scope #49540

Closed
wants to merge 5 commits into from

Conversation

LiviaMedeiros
Copy link
Contributor

d78a9da blocked on #49531

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@LiviaMedeiros LiviaMedeiros added the blocked PRs that are blocked by other issues or PRs. label Sep 7, 2023
@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Sep 7, 2023
doc/api/esm.md Show resolved Hide resolved
Comment on lines +48 to +52
fd = openSync(url);
readSync(fd, magic);
if (magic[0] === 0x00 && magic[1] === 0x61 && magic[2] === 0x73 && magic[3] === 0x6d) {
return 'wasm';
}
Copy link
Member

Choose a reason for hiding this comment

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

I get that we’re just reading the first four bytes here (I think?) but doesn’t doing this here mean that we open and read the file twice? Once to detect format and again later to actually run it?

Especially since this happens for every ESM entry point, even JavaScript ones, this feels like a performance regression that we should avoid.

Copy link
Contributor

Choose a reason for hiding this comment

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

This happens only for extensionless files, right? If that's the case, that seems absolutely OK as a tradeoff, the workaround is to use the .mjs extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct, the check is only for local extensionless files in module scope.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a tradeoff? Can’t we keep the file contents in memory once we have them, so that later on when the load hook runs it uses the contents from memory rather than refetching them from disk?

export function jsInitFn () {
strictEqual(state, 'JS Function Executed');
state = 'WASM Start Executed';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
}

@GeoffreyBooth
Copy link
Member

Closing now that #49974 has landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants