lib: refactor ES module loader for readability (JS side)#16579
Uh oh!
There was an error while loading. Please reload this page.
Conversation
addaleax added esm 
nodejs-github-bot added the errors addaleaxforce-pushed the esm-readability-js branch from d65e5bb to 5e5f1e1Compare
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 ;)
targos commented Oct 29, 2017
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.
addaleaxforce-pushed the esm-readability-js branch from 357541c to 32704fdCompare
addaleax commented Nov 3, 2017
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]>
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. |
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]>
9 participants
/cc @guybedford
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
esm