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

lib: refactor ES module loader for readability (JS side) #16579

Closed
wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

/cc @guybedford

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

esm

@addaleax addaleax added esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 29, 2017
@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Oct 29, 2017
]);
`;
// Create two modules: One whose exports are get- and set-able ('reflective'),
// and one which re-exports all these of these but additionally may
Copy link
Member

Choose a reason for hiding this comment

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

all of these

@@ -72,14 +100,20 @@ class Loader {
return { url, format };
}

// May create a new ModuleJob instance if one did not already exist.
async getModuleJob(specifier, parentURL = this.base) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Your comment makes me wonder if this would be better called getOrCreateModuleJob.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is fine, get kind of leaves the details open of how the returned module job got into existence :)

}
throw e;
}
this.module.instantiate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing to see this simplification. Not having to call instantiate for each module in the right order is a much nicer v8 API!

Copy link
Member Author

Choose a reason for hiding this comment

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

Just in case it wasn’t clear, we only had one call to instantiate() before as well, but maybe this is a bit more obvious now :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, it seems I was simply trying to post-justify my less elegant approach :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, didn’t know this was your code ;)

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Looks great to me.

@targos
Copy link
Member

targos commented Oct 29, 2017

this.dynamicInstantiate = undefined;
}

static get validFormats() {
return ['esm', 'cjs', 'builtin', 'addon', 'json', 'dynamic'];
Copy link
Member

Choose a reason for hiding this comment

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

This could be a static property instead of a getter, to avoid recreating such an array every time.

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, makes sense!

@addaleax
Copy link
Member Author

addaleax commented Nov 3, 2017

Landed in 14181a3

@addaleax addaleax closed this Nov 3, 2017
@addaleax addaleax deleted the esm-readability-js branch November 3, 2017 00:18
addaleax added a commit that referenced this pull request Nov 3, 2017
PR-URL: #16579
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
PR-URL: nodejs#16579
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
@gibfahn
Copy link
Member

gibfahn commented Nov 14, 2017

This conflicts in doc/api/errors.md on v8.x, @addaleax would you mind raising a backport PR? Should be an easy conflict to fix.

gibfahn pushed a commit that referenced this pull request Apr 12, 2018
PR-URL: #16579
Backport-PR-URL: #18085
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants