WIP: Resolve hook prototype #1

guybedford · 2017-09-03T11:26:00Z

I've done some initial work here on bringing in a resolve hook based on the signature resolve (specifier, parentURL) => Promise<{ url: URL, format: string }>, as in https://github.com/guybedford/node-resolve-hook.

In the process I've simplified the provider and module job architecture a bit. Providers are now simply functions instead of full class objects, with the finalize method being avoided by taking the return object from Module._load. I've also cut down on the number of URL serializations. There are a few other small tweaks as well, happy to discuss them further in the code.

The above optimizations could certainly be separated from the resolver work as well if you like to review them separately.

Would really value further feedback here, and if this kind of a direction might be suitable!

@Fishrock123

This follows the EPS an allows the node CLI to have ESM as an entry point. `node ./example.mjs`. A newer V8 is needed for `import()` so that is not included. `import.meta` is still in specification stage so that also is not included. Author: Bradley Farias <[email protected]> Author: Guy Bedford <[email protected]> Author: Jan Krems <[email protected]> fix eslint errors add missing include restore module.exports remove TryCatch Use anonymous namespace, fix compilation warnings Clean up string usage Fix c++ linting Allow unlinked cwd Fixes make test. fix indentation eslint fix more common whitespace usage, fix missing header include try to address Windows build failure throw no-base error in JS No need to go to C++ for this. Fix resolve module dir URL Fix module.load usage Makes CJS module loading work process.exit(1) when there is an error in ESM loading Add support for .mjs in test harness tools: support ESM in ESLint required-modules rule Start porting ESM tests Disable required-modules for basic mjs tests Avoid using auto for easy-to-mix up cases fix linter errors for new linter rules on `master` More linting use null as [[Prototype]] directly move src/loader/* out of its own directory fix more linting properly decode url encoding in pathnames Cleanup URL->file path conversion Error cases, add tests. lint fix module._cache regression move test out of fixtures nits on test properly realpath after getting path from URL reject module instantiation when binding throws call e.stack getter preserve-symlinks support remove unnecessary index file flag as harmony-modules lint ordering of lists add .mjs to coverage, ESM support appears buggy though simple ESM namespace keys test move logic for making URL from file path f test and fix snapshot timing of CJS use internal/errors fix bug with moduleProvider in CJS snapshot nits from @Fishrock123 linting gate

bmeck · 2017-09-04T20:53:11Z

lib/internal/loader/Loader.js

-    const request = resolveRequestUrl(this.base, specifier);
-    if (request.url.protocol !== 'file:') {
+  setModuleResolver(resolver) {
+    resolve = resolver;


accidental assignment to a const?

bmeck · 2017-09-04T20:55:57Z

lib/internal/loader/Loader.js

+  async getModuleJob(specifier, parentURLOrString = this.base) {
+    const { url, format } = await this.resolve(specifier, parentURLOrString);
+    const urlString = `${url}`;
+    let job = this.moduleMap.get(urlString);


please check using .has

This was done as a perf optimization to avoid repeated unnecessary dictionary lookups, but unsure how much it would affect things. Happy to change it.

bmeck · 2017-09-05T05:38:25Z

I think we are on the right track here. I would prefer to be overly aggressive in merging once the current PR lands so we can start getting feedback. In particular, I want browser spec feedback and will be reaching out to them. API likely will stay flexible until then.

Under the assumption that package level guards are placed instead of global hooks I think it is ok to merge as a global hook first and play with the APIs while guards are discussed.

I am not sure on node: or eval: protocols right now and need to think. I would prefer to avoid them if possible. For now, I think we can take this as an experimental ground and see how it works since I cannot see a great alternative. Removing node: and having builtins be special specifiers sounds sane; but I do not have a clear way to deal with vm style creation of things (we don't have any hooks into eval itself). The only idea for changing eval: is to declare the loader in the options passed to createScript etc., but that sounds like a guard that I am not sure is needed/desired.

If it is ok with you, I would like to push a few commits ontop this for nits/docs/tests in the next few days.

guybedford · 2017-09-05T11:28:56Z

Sounds great, thanks for taking the time to look at it. Feel free to add changes on top of this completely, and no need to worry about authorship either if histories get messed up. Will be very interesting to hear browser feedback.

Certainly the eval: would have to come later given an API for it - I guess that is the vm.createModuleContext or similar for modules, which would also be interesting to hear browser feedback on.

Then yes it sounds like we need to have a conversation about the hook scope in due course. I do take the position that this is an application-scoped boot-level hook, and as such should have it's own control, but there's definitely lot to flesh out and discuss in terms of the use cases and abuse cases too.

Currently when running the test without an internet connection there are two JavaScript test failures and one cctest. The cctest only fails on Mac as far as I know. (I've only tested using Mac and Linux thus far). This commit moves the two JavaScript tests to test/internet. The details for test_inspector_socket_server.cc: [ RUN ] InspectorSocketServerTest.FailsToBindToNodejsHost make[1]: *** [cctest] Segmentation fault: 11 make: *** [test] Error 2 lldb output: [ RUN ] InspectorSocketServerTest.FailsToBindToNodejsHost Process 63058 stopped * thread #1: tid = 0x7b175, 0x00007fff96d04384 * libsystem_info.dylib`_gai_simple + 87, queue = * 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, * address=0x0) frame #0: 0x00007fff96d04384 libsystem_info.dylib`_gai_simple + 87 libsystem_info.dylib`_gai_simple: -> 0x7fff96d04384 <+87>: movw (%rdx), %ax 0x7fff96d04387 <+90>: movw %ax, -0x2a(%rbp) 0x7fff96d0438b <+94>: movq %r13, -0x38(%rbp) 0x7fff96d0438f <+98>: movq 0x18(%rbp), %rcx (lldb) bt * thread #1: tid = 0x7b175, 0x00007fff96d04384 * libsystem_info.dylib`_gai_simple + 87, queue = * 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, * address=0x0) * frame #0: 0x00007fff96d04384 libsystem_info.dylib`_gai_simple + 87 frame #1: 0x00007fff96cfe98b libsystem_info.dylib`search_addrinfo + 179 frame #2: 0x00007fff96cfafef libsystem_info.dylib`si_addrinfo + 2255 frame nodejs#3: 0x00007fff96cfa67b libsystem_info.dylib`getaddrinfo + 179 frame nodejs#4: 0x00000001017d8888 cctest`uv__getaddrinfo_work(w=0x00007fff5fbfe210) + 72 at getaddrinfo.c:102 frame nodejs#5: 0x00000001017d880e cctest`uv_getaddrinfo(loop=0x000000010287cb80, req=0x00007fff5fbfe1c8, cb=0x0000000000000000, hostname="nodejs.org", service="0", hints=0x00007fff5fbfe268) + 734 at getaddrinfo.c:192 frame nodejs#6: 0x000000010171f781 cctest`node::inspector::InspectorSocketServer::Start(this=0x00007fff5fbfe658) + 801 at inspector_socket_server.cc:398 frame nodejs#7: 0x00000001016ed590 cctest`InspectorSocketServerTest_FailsToBindToNodejsHost_Test::TestBody(this=0x0000000105001fd0) + 288 at test_inspector_socket_server.cc:593 I'm not sure about the exact cause for this but when using a standalone c program to simulate this it seems like when the ai_flags `AI_NUMERICSERV` is set, which is done in inspector_socket_server.cc line 394, the servname (the port in the FailsToBindToNodejsHost test) is expected to be a numeric port string to avoid looking it up in /etc/services. When the port is 0 as is it was before this commit the segment fault occurs but not if it is non-zero. PR-URL: nodejs#16255 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Remove a pointless adapter frame by fixing up the function's formal parameter count. Before: frame #0: 0x000033257ea446d5 onParserExecute(...) frame #1: 0x000033257ea3b93f <adaptor> frame #2: 0x000033257ea41959 <internal> frame nodejs#3: 0x000033257e9840ff <entry> After: frame #0: 0x00000956287446d5 onParserExecute(...) frame #1: 0x0000095628741959 <internal> frame #2: 0x00000956286840ff <entry> PR-URL: nodejs#17693 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

Reverting this enables us to provide slower, but longer-lasting replacements for the deprecated APIs. Original commit message: Put back deleted V8_DEPRECATE_SOON methods This partially reverts https://chromium-review.googlesource.com/c/v8/v8/+/1177861, which deleted many V8_DEPRECATE_SOON methods rather than moving them to V8_DEPRECATED first. This puts them back and marks them V8_DEPRECATED. Note V8_DEPRECATED that were deleted in the same CL stay deleted. NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: v8:7786, v8:8240 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I00330036d957f98dab403465b25e30d8382aac22 Reviewed-on: https://chromium-review.googlesource.com/1251422 Commit-Queue: Dan Elphick <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Michael Hablich <[email protected]> Cr-Commit-Position: refs/branch-heads/7.0@{nodejs#49} Cr-Branched-From: 6e2adae6f7f8e891cfd01f3280482b20590427a6-refs/heads/7.0.276@{#1} Cr-Branched-From: bc08a8624cbbea7a2d30071472bc73ad9544eadf-refs/heads/master@{nodejs#55424} Refs: v8/v8@9136dd8 Refs: nodejs#23122 PR-URL: nodejs#23158 Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

bmeck added 5 commits August 31, 2017 11:37

warn on using ESM loader

918e936

basic docs

118d18a

doc nits

762cf03

doc nits

1adf7cb

guybedford force-pushed the resolve-hook branch 2 times, most recently from 133d610 to 4c784a0 Compare September 3, 2017 16:49

functional providers, format separation, resolve hook prototype

6e328d0

guybedford force-pushed the resolve-hook branch 2 times, most recently from 0e15dbb to 13c8e4a Compare September 3, 2017 17:06

stricter signature, parent url tweaks

5496f77

guybedford force-pushed the resolve-hook branch from 13c8e4a to 5496f77 Compare September 3, 2017 17:10

bmeck reviewed Sep 4, 2017

View reviewed changes

const fix

31aa66f

bmeck force-pushed the initial-pr branch 3 times, most recently from 75eebf3 to 11844b0 Compare September 7, 2017 20:18

guybedford changed the base branch from initial-pr to master September 8, 2017 16:28

guybedford changed the base branch from master to initial-pr September 8, 2017 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Resolve hook prototype #1

WIP: Resolve hook prototype #1

guybedford commented Sep 3, 2017 •

edited

Loading

bmeck Sep 4, 2017

bmeck Sep 4, 2017 •

edited

Loading

guybedford Sep 5, 2017

bmeck commented Sep 5, 2017

guybedford commented Sep 5, 2017

WIP: Resolve hook prototype #1

Are you sure you want to change the base?

WIP: Resolve hook prototype #1

Conversation

guybedford commented Sep 3, 2017 • edited Loading

bmeck Sep 4, 2017

Choose a reason for hiding this comment

bmeck Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

guybedford Sep 5, 2017

Choose a reason for hiding this comment

bmeck commented Sep 5, 2017

guybedford commented Sep 5, 2017

guybedford commented Sep 3, 2017 •

edited

Loading

bmeck Sep 4, 2017 •

edited

Loading