lib: refactor ES module loader for readability (JS side) #16579
Conversation
addaleax
added
esm
nodejs-github-bot
added
the
errors
addaleax
force-pushed
the
esm-readability-js
branch
from
d65e5bb
to
5e5f1e1
Compare
lib/internal/loader/ModuleWrap.js
Outdated
| ]); | ||
| `; | ||
| // Create two modules: One whose exports are get- and set-able ('reflective'), | ||
| // and one which re-exports all these of these but additionally may |
| @@ -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) { | |||
There was a problem hiding this comment.
Your comment makes me wonder if this would be better called getOrCreateModuleJob.
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
Amazing to see this simplification. Not having to call instantiate for each module in the right order is a much nicer v8 API!
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Huh, it seems I was simply trying to post-justify my less elegant approach :)
There was a problem hiding this comment.
Heh, didn’t know this was your code ;)
lib/internal/loader/Loader.js
Outdated
| this.dynamicInstantiate = undefined; | ||
| } | ||
|
|
||
| static get validFormats() { | ||
| return ['esm', 'cjs', 'builtin', 'addon', 'json', 'dynamic']; |
There was a problem hiding this comment.
This could be a static property instead of a getter, to avoid recreating such an array every time.
addaleax
force-pushed
the
esm-readability-js
branch
from
357541c
to
32704fd
Compare
|
Landed in 14181a3 |
PR-URL: #16579 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#16579 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
|
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. |
PR-URL: #16579 Backport-PR-URL: #18085 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
/cc @guybedford
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
esm