lib: refactor ES module loader for readability (JS side) #16579
Conversation
d65e5bb to
5e5f1e1
Compare
lib/internal/loader/ModuleWrap.js
Outdated
lib/internal/loader/Loader.js
Outdated
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 :)
lib/internal/loader/ModuleJob.js
Outdated
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
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.
357541c to
32704fd
Compare
|
Landed in 14181a3 |
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>
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>
|
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. |
/cc @guybedford
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
esm