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

module: ensure format is never null with experimental-detect-module #53012

Closed
wants to merge 3 commits into from
Closed

module: ensure format is never null with experimental-detect-module #53012

wants to merge 3 commits into from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented May 15, 2024 •
edited

This change ensures that the format is consistently defined when experimental-detect-module is enabled, allowing tests to pass in both experimental detection and standard Node environments.

(Fourth PR is the charm)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels May 15, 2024
@RedYetiDev RedYetiDev added loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem. labels May 15, 2024
@RedYetiDev RedYetiDev changed the title module: ensure format is never null module: ensure format is never null with experimental-detect-module May 15, 2024
GeoffreyBooth
GeoffreyBooth reviewed May 15, 2024
View reviewed changes
lib/internal/modules/esm/load.js Outdated
@@ -141,7 +141,9 @@ async function defaultLoad(url, context = kEmptyObject) {
}

validateAttributes(url, format, importAttributes);

if (format == null && getOptionValue('--experimental-detect-module')) {
Copy link
Member

Choose a reason for hiding this comment

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

And what if we detected that the module is ESM? Then format would be 'module' already?

We should probably add a comment listing the conditions when format == null might happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

Essentially, if we don't know the format and we should autodetect it, assume it is commonjs

@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 15, 2024 •
edited

I'd like to apologize for the confusion around these changes, I've tried several attempts, which made this really confusing.

Essentially, when the --experimental-detect-module flag is enabled, and it cannot automatically detect the module type, the get_format will return a Symbol, which will then tell the resolver to set format to commonjs.

The symbol name kFormatHasNoSource refers to the fact that the only time it will be used is when the format cannot be automatically detetected because it has no source loaded.

GeoffreyBooth
GeoffreyBooth reviewed May 15, 2024
View reviewed changes
src/node_options.h Outdated Show resolved Hide resolved
lib/internal/modules/esm/get_format.js
@@ -224,4 +226,5 @@ module.exports = {
defaultGetFormatWithoutErrors,
extensionFormatMap,
extname,
kFormatHasNoSource,
Copy link
Member

Choose a reason for hiding this comment

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

Did you forget to commit something? I don’t see when you’re ever setting format to be this symbol.

We don’t want this symbol leaking into user hooks. There shouldn’t be a user load hook that ever sees format set to this symbol.

@RedYetiDev @GeoffreyBooth
Update src/node_options.h
0b70b58 
Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>
@RedYetiDev RedYetiDev marked this pull request as draft May 15, 2024 22:40
@RedYetiDev
Copy link
Member Author

Everything was working, but now something went wrong, strange, I need to debug

@RedYetiDev RedYetiDev closed this May 15, 2024
@RedYetiDev RedYetiDev deleted the never-null-format branch May 15, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth left review comments

@aduh95 aduh95 Awaiting requested review from aduh95

Assignees
No one assigned
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@RedYetiDev @nodejs-github-bot @GeoffreyBooth