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

module: resolve and instantiate loader pipeline hooks #15445

Closed
wants to merge 1 commit into
base: master
from

Conversation

@guybedford
Contributor

guybedford commented Sep 17, 2017

This PR provides an implementation of a hooking mechanism worked on with some early feedback from @bmeck.

Usage

The es module loading pipeline is hooked via a --loader flag to Node, which specifies a file containing a loader definition.

For example usage:

Motivation

This work is built to follow principles of extensibility - allowing users (framework and platform developers for this feature) to customize their own hooks for their use case needs. The core ecosystem conventions for loading modules in NodeJS will remain the ones that NodeJS sets itself by the ecosystem popularity due to interop requirements.

The goal here though is that through having the ability for users to experiment with loader features will allow the most useful and popular features to potentially be merged back into NodeJS core to extend the ecosystem conventions. So roughly the extensible manifesto thinking.

I believe enabling these use cases is important to avoid possible ecosystem fragmentation through other means.

The examples above show common use cases, and these hooks are also sufficient to implement the package.json "module" property resolution concept using a loader (nodejs/node-eps#60).

Architecture

The hook architecture here is inspired by learnings from the WhatWG loader work, while aiming to cut down the scope of hooking to a bare minimum API surface area, as can be seen from the examples.

The loader itself is given control of module resolution, of the form:

resolve (specifier: string, parentModuleUrl: string) => { url: string, format: string }

Where the "format" returned is "esm" for the ES module loader, "cjs" for the traditional cjs loader, "builtin" to refer to a NodeJS core module, "json" for json and "addon" for a Node binary addon. In future this could be extended for new module formats (ie wasm, binary ast).

This resolver follows exactly what has always been the resolve hook in the module loader specifications, except instead of just returning a string URL, it returns the URL along with the format. Other information could also possibly be returned on this object in future including for example integrity metadata.

When the format returned is "dynamic", then we delegate loading to a dynamicInstantiate hook, hooked in the same way as resolve:

async dynamicInstantiate (url: string) => ({
  exports: string[],
  execute: (exports) => {
    // get and set functions provided for pre-allocated export names
    exports.x.set('asdf');
  }
})

This API is designed to enable the above example use cases, while ensuring that loader internals are kept private, to ensure that what is hooked is only what is intended to be hooked. For example, none of ModuleWrap is exposed.

Feedback very welcome!

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

esmodules

@guybedford guybedford changed the title from Loader pipeline resolver and loader hooking to Module pipeline resolver and loader hooking Sep 17, 2017

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe Sep 17, 2017

Member

@guybedford what might it look like to extend on the hooks available, such that a modified source-file could be stored in the cache?

I'm interested in how we'll continue providing test-coverage for .mjs files (which I believe circumvent the current require.extensions API -- this seems like it could be a step in the direction of what I'd need to get Istnabul working with .mjs?

Member

bcoe commented Sep 17, 2017

@guybedford what might it look like to extend on the hooks available, such that a modified source-file could be stored in the cache?

I'm interested in how we'll continue providing test-coverage for .mjs files (which I believe circumvent the current require.extensions API -- this seems like it could be a step in the direction of what I'd need to get Istnabul working with .mjs?

@guybedford

This comment has been minimized.

Show comment
Hide comment
@guybedford

guybedford Sep 17, 2017

Contributor

@bcoe one option might be to add a separate 'fetch' hook to such a loader object alongside the 'resolve' here which could then add instrumentation. Happy to prototype it if there's interest.

Contributor

guybedford commented Sep 17, 2017

@bcoe one option might be to add a separate 'fetch' hook to such a loader object alongside the 'resolve' here which could then add instrumentation. Happy to prototype it if there's interest.

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe Sep 17, 2017

Member

@guybedford I'm very interested. The built-in coverage in V8 doesn't seem to quite be over the finish line yet, so this would be a great transitional step for continuing to support test-coverage with Istanbul.

Member

bcoe commented Sep 17, 2017

@guybedford I'm very interested. The built-in coverage in V8 doesn't seem to quite be over the finish line yet, so this would be a great transitional step for continuing to support test-coverage with Istanbul.

@bcoe bcoe referenced this pull request Sep 17, 2017

Closed

build: run es-module tests in ci environment #15276

3 of 3 tasks complete

@mscdex mscdex added the ES Modules label Sep 17, 2017

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe Sep 17, 2017

Member

@guybedford @bmeck any chance we could also allow you to attach a loader in a way similar to module.extensions. Perhaps module.loader? This would make the implementation way easier for tools like Istanbul. Then I could simply check whether module.loader exists, and instrument code accordingly.

Member

bcoe commented Sep 17, 2017

@guybedford @bmeck any chance we could also allow you to attach a loader in a way similar to module.extensions. Perhaps module.loader? This would make the implementation way easier for tools like Istanbul. Then I could simply check whether module.loader exists, and instrument code accordingly.

@guybedford

This comment has been minimized.

Show comment
Hide comment
@guybedford

guybedford Sep 18, 2017

Contributor

@bcoe something like module.loaderHooks = { fetch, resolve } or module.registerHook('resolve', resolve) is possible, but there is the async load timing issue here in that the boot of the application will not run through the hooks in that case. The boot for such a scenario then might look like:

import module from 'module';
import { customResolve, customFetch } from 'custom-loader';

module.registerHook('resolve', customResolve);
module.registerHook('fetch', customFetch);

// now kick off the dynamic import hooking the main application tree load
import('./entry-point.mjs');

This was actually roughly what I had for the original API here, but after discussion with @bmeck we adopted a flag rather here to avoid some of the above issues. I'm still open to either approach.

Contributor

guybedford commented Sep 18, 2017

@bcoe something like module.loaderHooks = { fetch, resolve } or module.registerHook('resolve', resolve) is possible, but there is the async load timing issue here in that the boot of the application will not run through the hooks in that case. The boot for such a scenario then might look like:

import module from 'module';
import { customResolve, customFetch } from 'custom-loader';

module.registerHook('resolve', customResolve);
module.registerHook('fetch', customFetch);

// now kick off the dynamic import hooking the main application tree load
import('./entry-point.mjs');

This was actually roughly what I had for the original API here, but after discussion with @bmeck we adopted a flag rather here to avoid some of the above issues. I'm still open to either approach.

Show outdated Hide outdated src/node.cc
READONLY_BOOLEAN_PROPERTY("experimentalModules");
if (!config_userland_loader.empty()) {

This comment has been minimized.

@jasnell

jasnell Sep 18, 2017

Member

thank you for using node_config to expose this :-)

@jasnell

jasnell Sep 18, 2017

Member

thank you for using node_config to expose this :-)

This comment has been minimized.

@guybedford

guybedford Sep 18, 2017

Contributor

That is all @bmeck!

@guybedford

guybedford Sep 18, 2017

Contributor

That is all @bmeck!

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 18, 2017

Member

Awesome to see this @guybedford ... left a few initial review comments but will need to play with this a bit more before signing off. Thank you for working on this

Member

jasnell commented Sep 18, 2017

Awesome to see this @guybedford ... left a few initial review comments but will need to play with this a bit more before signing off. Thank you for working on this

@bmeck

This comment has been minimized.

Show comment
Hide comment
@bmeck

bmeck Sep 18, 2017

Member

I'd like to leave things as declarative instead of API based if possible for now. require.extensions and people monkey patching module.* have cause a lot of lock-in that we cannot undo in CJS in part because of how hard it is to tell when something is being used, and in part because of the power of making them JS object properties.

As for a fetch transform, I have several ideas on how to accomplish similar ideas without introducing that transform:

  1. make a file on disk w/ mutated source and resolve to that.
  2. we introduce blob backed URL.createObjectURL and use that during resolution.

These can be worked through in the browser if done carefully; I am not convinced we should introduce fetch hooks in Node without creating a story for network backed modules and security of them. I also would like some involvement from browser people who are creating multiple different execution workers like ServiceWorker in order to do these tasks.

For code coverage I know V8 now has that instrumented in the codegen, so perhaps using that and adding missing features would be easier over time than trying to have hooks. Especially since we don't have a composition mechanism for these hooks currently.

This PR looks good even though I think we have more than enough hook area already and should not expand the scope of the PR. Overall, I think minimal hooks is the best approach in the beginning as I still think these hooks will change as we use them / won't look exactly the same by the time they are released.

feature opinion notes
Resolve hook +1 need composition eventually
Evaluation hook +1 would like more research over time into properly managing live bindings for core (e.g. https://github.com/bmeck/node/tree/named-export-core using a Proxy)
Fetch hook -1 seems like alternates
JS based API -1 has been a source of pain in past, no clear use case need for it vs declarative
Member

bmeck commented Sep 18, 2017

I'd like to leave things as declarative instead of API based if possible for now. require.extensions and people monkey patching module.* have cause a lot of lock-in that we cannot undo in CJS in part because of how hard it is to tell when something is being used, and in part because of the power of making them JS object properties.

As for a fetch transform, I have several ideas on how to accomplish similar ideas without introducing that transform:

  1. make a file on disk w/ mutated source and resolve to that.
  2. we introduce blob backed URL.createObjectURL and use that during resolution.

These can be worked through in the browser if done carefully; I am not convinced we should introduce fetch hooks in Node without creating a story for network backed modules and security of them. I also would like some involvement from browser people who are creating multiple different execution workers like ServiceWorker in order to do these tasks.

For code coverage I know V8 now has that instrumented in the codegen, so perhaps using that and adding missing features would be easier over time than trying to have hooks. Especially since we don't have a composition mechanism for these hooks currently.

This PR looks good even though I think we have more than enough hook area already and should not expand the scope of the PR. Overall, I think minimal hooks is the best approach in the beginning as I still think these hooks will change as we use them / won't look exactly the same by the time they are released.

feature opinion notes
Resolve hook +1 need composition eventually
Evaluation hook +1 would like more research over time into properly managing live bindings for core (e.g. https://github.com/bmeck/node/tree/named-export-core using a Proxy)
Fetch hook -1 seems like alternates
JS based API -1 has been a source of pain in past, no clear use case need for it vs declarative
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 18, 2017

Member

I'm not sure when this is targeted to land, but this seems very early. I would hope this doesn't end up in Node core at least until the ecosystem has had some time to settle down and explore alternatives and prove that this particular API shape is the best, and that the use cases it addresses are actually important enough to add such a powerful, fragmentation-encouraging primitive which cannot be taken back.

General API comment: please never return or accept URL objects; see https://url.spec.whatwg.org/#url-apis-elsewhere .

On the actual capabilities here, I think the resolve hook is straightforward enough (although may end up mismatching browsers if they need something more declarative, as is currently the thinking). The loader object is much scarier and is nowhere near what we would currently contemplate exposing in browsers (as essentially we are trying to stick with Source Text Module Records, so any hook I can imagine would be just a fetch hook, i.e. service workers).

Member

domenic commented Sep 18, 2017

I'm not sure when this is targeted to land, but this seems very early. I would hope this doesn't end up in Node core at least until the ecosystem has had some time to settle down and explore alternatives and prove that this particular API shape is the best, and that the use cases it addresses are actually important enough to add such a powerful, fragmentation-encouraging primitive which cannot be taken back.

General API comment: please never return or accept URL objects; see https://url.spec.whatwg.org/#url-apis-elsewhere .

On the actual capabilities here, I think the resolve hook is straightforward enough (although may end up mismatching browsers if they need something more declarative, as is currently the thinking). The loader object is much scarier and is nowhere near what we would currently contemplate exposing in browsers (as essentially we are trying to stick with Source Text Module Records, so any hook I can imagine would be just a fetch hook, i.e. service workers).

@Fishrock123

left some comments :)

Show outdated Hide outdated lib/internal/errors.js
Show outdated Hide outdated lib/internal/errors.js
Show outdated Hide outdated lib/internal/loader/ModuleRequest.js
Show outdated Hide outdated lib/internal/loader/ModuleRequest.js
Show outdated Hide outdated lib/internal/loader/ModuleRequest.js
Show outdated Hide outdated src/node.cc
@guybedford

This comment has been minimized.

Show comment
Hide comment
@guybedford

guybedford Sep 18, 2017

Contributor

Thanks so much for all feedback so far. I've updated the description above to match the latest updates:

  • I've renamed loader to format in the resolver hook, which is now fully consistent with being interpreted as the module format of the module being loaded, and no longer accepts a loader function. Hopefully this simplifies the resolve hook and makes it easier to see it as something that might be compatible with browsers too.
  • To support the previous loader option, when the "dynamic" format is returned, module loading is delegated to a separate dynamicInstantiate hook which takes the place of the functional loader option.
  • The hooks no longer accept or return URL objects, but work with string URLs only.
Contributor

guybedford commented Sep 18, 2017

Thanks so much for all feedback so far. I've updated the description above to match the latest updates:

  • I've renamed loader to format in the resolver hook, which is now fully consistent with being interpreted as the module format of the module being loaded, and no longer accepts a loader function. Hopefully this simplifies the resolve hook and makes it easier to see it as something that might be compatible with browsers too.
  • To support the previous loader option, when the "dynamic" format is returned, module loading is delegated to a separate dynamicInstantiate hook which takes the place of the functional loader option.
  • The hooks no longer accept or return URL objects, but work with string URLs only.
Show outdated Hide outdated lib/module.js
@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Sep 19, 2017

Member

Ok, so @bcoe asked me to chime in:

My primary use case for a custom loader is mocks. I currently maintain require-inject which fiddles with the cache to allow tests to replace modules with their own code. How it does this is not something I'd want to see replicated as it's very much a hack, but the functionality is something I don't want to lose with ESM.

It looks like a loader API + Module.import() or a v8 import() would meet my needs.

An interpreter flag might be workable (because these are tests) but would be awkward, and some version of import() would still be needed. (Currently I can run my tests with node ./path/to/test.js, having to run it as node --with-loader=./path/to/loader.js ./path/to/test.mjs would basically make that kind of ad hoc command line use too awkward to use.)

Member

iarna commented Sep 19, 2017

Ok, so @bcoe asked me to chime in:

My primary use case for a custom loader is mocks. I currently maintain require-inject which fiddles with the cache to allow tests to replace modules with their own code. How it does this is not something I'd want to see replicated as it's very much a hack, but the functionality is something I don't want to lose with ESM.

It looks like a loader API + Module.import() or a v8 import() would meet my needs.

An interpreter flag might be workable (because these are tests) but would be awkward, and some version of import() would still be needed. (Currently I can run my tests with node ./path/to/test.js, having to run it as node --with-loader=./path/to/loader.js ./path/to/test.mjs would basically make that kind of ad hoc command line use too awkward to use.)

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Sep 19, 2017

Member

@iarna

having to run it as node --with-loader=./path/to/loader.js ./path/to/test.mjs would basically make that kind of ad hoc command line use too awkward to use.)

I'd assume --loader package-name works too, which is similar to how I'm seeing folks use @std/esm (using node -r @std/esm ./path/to/test.js). Would that smooth things over?

Member

jdalton commented Sep 19, 2017

@iarna

having to run it as node --with-loader=./path/to/loader.js ./path/to/test.mjs would basically make that kind of ad hoc command line use too awkward to use.)

I'd assume --loader package-name works too, which is similar to how I'm seeing folks use @std/esm (using node -r @std/esm ./path/to/test.js). Would that smooth things over?

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe Sep 19, 2017

Member

@jdalton @iarna

I'd assume --loader package-name works too, which is similar to how I'm seeing folks use @std/esm (using node -r @std/esm ./path/to/test.js). Would that smooth things over?

This becomes a bit awkward for writing testing tools like nyc. Users will often spawn a subprocess in a cwd that potentially has its own node_modules folder. So, unless you were going to require that someone install this category of tool globally (which feels gross), you end up doing a lot of bookkeeping to ensure that spawned subprocesses have the appropriate flags.

This is a similar concern I have with trying to facilitate test-coverage functionality using V8's built-in coverage. Which is even more of a Rube Goldberg machine, as you need to spawn a process with --inspect, track which port has been used, etc.

tldr; it's a lot easier if there's a programatic way hook into this functionality in a vanilla Node process .

Member

bcoe commented Sep 19, 2017

@jdalton @iarna

I'd assume --loader package-name works too, which is similar to how I'm seeing folks use @std/esm (using node -r @std/esm ./path/to/test.js). Would that smooth things over?

This becomes a bit awkward for writing testing tools like nyc. Users will often spawn a subprocess in a cwd that potentially has its own node_modules folder. So, unless you were going to require that someone install this category of tool globally (which feels gross), you end up doing a lot of bookkeeping to ensure that spawned subprocesses have the appropriate flags.

This is a similar concern I have with trying to facilitate test-coverage functionality using V8's built-in coverage. Which is even more of a Rube Goldberg machine, as you need to spawn a process with --inspect, track which port has been used, etc.

tldr; it's a lot easier if there's a programatic way hook into this functionality in a vanilla Node process .

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Sep 19, 2017

Member

Users will often spawn a subprocess in a cwd that potentially has its own node_modules folder. So, unless you were going to require that someone install this category of tool globally (which feels gross)

I don't have a ton of experience with all that. I know that using tools like AVA, which forks processes, I was able to enable @std/esm by tying into their require option. Which is to say if AVA can track requires (and I'd guess flags too) couldn't nyc and others too?

Member

jdalton commented Sep 19, 2017

Users will often spawn a subprocess in a cwd that potentially has its own node_modules folder. So, unless you were going to require that someone install this category of tool globally (which feels gross)

I don't have a ton of experience with all that. I know that using tools like AVA, which forks processes, I was able to enable @std/esm by tying into their require option. Which is to say if AVA can track requires (and I'd guess flags too) couldn't nyc and others too?

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe Sep 19, 2017

Member

@jdalton

I don't have a ton of experience with all that. I know that using tools like AVA, which forks processes, I was able to enable @std/esm by tying into their require option (which is serialized as a JSON string process.argv arg). Which is to say if AVA can do it couldn't nyc and others too?

nyc already does a lot of this, passing around state so that subprocesses can when spawned have their require.extensions patched (so that including a source-file results in it being instrumented by Istanbul).

My concern is that this flow is already very complex and bug prone. As it stands right now the new module system makes writing a development tool like this even more difficult.

EDIT: tldr; it feels like there's an opportunity to make the API for building features like this less painful, since the loader implementation is a greenfield right now. So, I don't see it as a question of whether it's possible, it's a question of can we reduce the burden placed on folks developing tools for Node.js.

Member

bcoe commented Sep 19, 2017

@jdalton

I don't have a ton of experience with all that. I know that using tools like AVA, which forks processes, I was able to enable @std/esm by tying into their require option (which is serialized as a JSON string process.argv arg). Which is to say if AVA can do it couldn't nyc and others too?

nyc already does a lot of this, passing around state so that subprocesses can when spawned have their require.extensions patched (so that including a source-file results in it being instrumented by Istanbul).

My concern is that this flow is already very complex and bug prone. As it stands right now the new module system makes writing a development tool like this even more difficult.

EDIT: tldr; it feels like there's an opportunity to make the API for building features like this less painful, since the loader implementation is a greenfield right now. So, I don't see it as a question of whether it's possible, it's a question of can we reduce the burden placed on folks developing tools for Node.js.

@bmeck

This comment has been minimized.

Show comment
Hide comment
@bmeck

bmeck Sep 19, 2017

Member

@bcoe

This becomes a bit awkward for writing testing tools like nyc. Users will often spawn a subprocess in a cwd that potentially has its own node_modules folder. So, unless you were going to require that someone install this category of tool globally (which feels gross), you end up doing a lot of bookkeeping to ensure that spawned subprocesses have the appropriate flags.

You can use the NODE_OPTIONS environment variable to intercept the subprocess and attach a --loader argument. Is that sufficient?

This is a similar concern I have with trying to facilitate test-coverage functionality using V8's built-in coverage. Which is even more of a Rube Goldberg machine, as you need to spawn a process with --inspect, track which port has been used, etc.

This seems like we can improve that workflow rather than making a more complex hook system.

My concern is that this flow is already very complex and bug prone. As it stands right now the new module system makes writing a development tool like this even more difficult.

This is exactly why I want to work on a minimal API and iterate rather than trying to recreate the old workflows. If we focus on specific aspects instead of trying to solve all the use cases at once I think we will not suffer the previous problems with standardizing loaders.

Member

bmeck commented Sep 19, 2017

@bcoe

This becomes a bit awkward for writing testing tools like nyc. Users will often spawn a subprocess in a cwd that potentially has its own node_modules folder. So, unless you were going to require that someone install this category of tool globally (which feels gross), you end up doing a lot of bookkeeping to ensure that spawned subprocesses have the appropriate flags.

You can use the NODE_OPTIONS environment variable to intercept the subprocess and attach a --loader argument. Is that sufficient?

This is a similar concern I have with trying to facilitate test-coverage functionality using V8's built-in coverage. Which is even more of a Rube Goldberg machine, as you need to spawn a process with --inspect, track which port has been used, etc.

This seems like we can improve that workflow rather than making a more complex hook system.

My concern is that this flow is already very complex and bug prone. As it stands right now the new module system makes writing a development tool like this even more difficult.

This is exactly why I want to work on a minimal API and iterate rather than trying to recreate the old workflows. If we focus on specific aspects instead of trying to solve all the use cases at once I think we will not suffer the previous problems with standardizing loaders.

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe Sep 19, 2017

Member

@bmeck

That's a really thoughtful point of view; mainly I just want to make sure we have examples of how we'd facilitate use-cases like dependency injection, coverage, and transpilation, before ES modules aren't behind a flag... I think if we can nail these three categories of common developer tools, we'd know the API changes are looking really healthy.

Member

bcoe commented Sep 19, 2017

@bmeck

That's a really thoughtful point of view; mainly I just want to make sure we have examples of how we'd facilitate use-cases like dependency injection, coverage, and transpilation, before ES modules aren't behind a flag... I think if we can nail these three categories of common developer tools, we'd know the API changes are looking really healthy.

@bmeck

This comment has been minimized.

Show comment
Hide comment
@bmeck

bmeck Sep 19, 2017

Member

@bcoe Dependency injection won't work like in other languages where it can change over time due to idempotency of ESM for any given specifier. The spec does not allow things like:

await import('thingFactory') != await import('thingFactory');

You can pass in data via query params as needed and that will give you a clean local module map that can change what thingFactory is. Can't re-use the same module though.

I'm not going to block unflagging on transpilation as they can be done ahead of time. However, various workflows like needing a way to get coverage is certainly blocking. If that is done ahead of time and works I would not see it as a blocker.

Member

bmeck commented Sep 19, 2017

@bcoe Dependency injection won't work like in other languages where it can change over time due to idempotency of ESM for any given specifier. The spec does not allow things like:

await import('thingFactory') != await import('thingFactory');

You can pass in data via query params as needed and that will give you a clean local module map that can change what thingFactory is. Can't re-use the same module though.

I'm not going to block unflagging on transpilation as they can be done ahead of time. However, various workflows like needing a way to get coverage is certainly blocking. If that is done ahead of time and works I would not see it as a blocker.

Show outdated Hide outdated lib/module.js

addaleax added a commit to addaleax/ayo that referenced this pull request Oct 12, 2017

module: resolve and instantiate loader pipeline hooks
This enables a --loader flag for Node, which can provide custom
"resolve" and "dynamicInstantiate" methods for custom ES module
loading.

In the process, module providers have been converted from classes
into functions and the module APIs have been made to pass URL strings
over objects.

PR-URL: nodejs/node#15445
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Oct 12, 2017

Member

Unfortunately, it appears this landed without a CI run 😱

...and it fails on Raspberry Pi devices:

The test added here has started failing on Raspberry Pi devices.

https://ci.nodejs.org/job/node-test-binary-arm/10851/RUN_SUBSET=4,label=pi1-raspbian-wheezy/console

not ok 249 es-module/test-esm-addon
  ---
  duration_ms: 2.65
  severity: fail
  stack: |-
    (node:29865) ExperimentalWarning: The ESM module loader is experimental.
    Error: module ../addons/hello-world/build/Release/binding.node not found
        at module.exports (internal/loader/search.js:14:12)
        at exports.resolve (internal/loader/ModuleRequest.js:91:13)
        at Loader.resolve (internal/loader/Loader.js:50:40)
        at Loader.getModuleJob (internal/loader/Loader.js:78:40)
        at ModuleWrap.module.link (internal/loader/ModuleJob.js:27:25)
        at linked (internal/loader/ModuleJob.js:25:19)
        at <anonymous>
Member

Trott commented Oct 12, 2017

Unfortunately, it appears this landed without a CI run 😱

...and it fails on Raspberry Pi devices:

The test added here has started failing on Raspberry Pi devices.

https://ci.nodejs.org/job/node-test-binary-arm/10851/RUN_SUBSET=4,label=pi1-raspbian-wheezy/console

not ok 249 es-module/test-esm-addon
  ---
  duration_ms: 2.65
  severity: fail
  stack: |-
    (node:29865) ExperimentalWarning: The ESM module loader is experimental.
    Error: module ../addons/hello-world/build/Release/binding.node not found
        at module.exports (internal/loader/search.js:14:12)
        at exports.resolve (internal/loader/ModuleRequest.js:91:13)
        at Loader.resolve (internal/loader/Loader.js:50:40)
        at Loader.getModuleJob (internal/loader/Loader.js:78:40)
        at ModuleWrap.module.link (internal/loader/ModuleJob.js:27:25)
        at linked (internal/loader/ModuleJob.js:25:19)
        at <anonymous>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Oct 12, 2017

Member

Oh, wait, I'm totally wrong, it had CI, even passing CI. (I searched for CI: rather than just CI because I'm a chump.) Sorry about that.

OK, so wonder what changed to cause the rash of failures all of a sudden...

Member

Trott commented Oct 12, 2017

Oh, wait, I'm totally wrong, it had CI, even passing CI. (I searched for CI: rather than just CI because I'm a chump.) Sorry about that.

OK, so wonder what changed to cause the rash of failures all of a sudden...

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe Oct 12, 2017

Member

@bmeck where would be a good place to start (or continue) a discussion about createObjectURL; I think this would be the most promising short term solution for keeping nyc/Istanbul working as we start to move some of the Node core modules over to .mjs.

Member

bcoe commented Oct 12, 2017

@bmeck where would be a good place to start (or continue) a discussion about createObjectURL; I think this would be the most promising short term solution for keeping nyc/Istanbul working as we start to move some of the Node core modules over to .mjs.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Oct 12, 2017

Member

Talking with @refack in IRC. The issue is indeed with this change but CI didn't catch it because of a bug in Makefile that caused the test to be omitted on CI.

Most relevant bit:

Yeah. This can't work on the ARM

import binding from '../addons/hello-world/build/Release/binding.node';

And is also build-order-sensitive on all other platforms

Member

Trott commented Oct 12, 2017

Talking with @refack in IRC. The issue is indeed with this change but CI didn't catch it because of a bug in Makefile that caused the test to be omitted on CI.

Most relevant bit:

Yeah. This can't work on the ARM

import binding from '../addons/hello-world/build/Release/binding.node';

And is also build-order-sensitive on all other platforms

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott
Member

Trott commented Oct 12, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Oct 12, 2017

Member

Yeah. This can't work on the ARM
And is also build-order-sensitive on all other platforms

For reference the arm-fanned job runs the addons suite on a different machine than any of the JS tests, so there is no compilation of the hello-world addon before test/es-module/test-esm-addon.mjs is run. (and also on the other platforms it's only by chance that it is done before this test is run).
tl;dr is should be moved to test/addons/hello-world/

Member

refack commented Oct 12, 2017

Yeah. This can't work on the ARM
And is also build-order-sensitive on all other platforms

For reference the arm-fanned job runs the addons suite on a different machine than any of the JS tests, so there is no compilation of the hello-world addon before test/es-module/test-esm-addon.mjs is run. (and also on the other platforms it's only by chance that it is done before this test is run).
tl;dr is should be moved to test/addons/hello-world/

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Oct 13, 2017

Member

Proposed fix in #16174

Member

Trott commented Oct 13, 2017

Proposed fix in #16174

@bcoe bcoe referenced this pull request Oct 13, 2017

Open

Implement createObjectURL/Blob from File API #16167

0 of 5 tasks complete

targos added a commit that referenced this pull request Oct 18, 2017

module: resolve and instantiate loader pipeline hooks
This enables a --loader flag for Node, which can provide custom
"resolve" and "dynamicInstantiate" methods for custom ES module
loading.

In the process, module providers have been converted from classes
into functions and the module APIs have been made to pass URL strings
over objects.

PR-URL: #15445
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

targos added a commit that referenced this pull request Oct 18, 2017

module: resolve and instantiate loader pipeline hooks
This enables a --loader flag for Node, which can provide custom
"resolve" and "dynamicInstantiate" methods for custom ES module
loading.

In the process, module providers have been converted from classes
into functions and the module APIs have been made to pass URL strings
over objects.

PR-URL: #15445
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

azz added a commit to azz/node that referenced this pull request Oct 22, 2017

doc: document resolve hook formats
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

Refs: nodejs#15445

@azz azz referenced this pull request Oct 22, 2017

Closed

doc: document resolve hook formats #16375

2 of 2 tasks complete

azz added a commit to azz/node that referenced this pull request Oct 22, 2017

doc: document resolve hook formats
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

Refs: nodejs#15445

azz added a commit to azz/node that referenced this pull request Oct 22, 2017

doc: document resolve hook formats
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

Refs: nodejs#15445

MylesBorins added a commit that referenced this pull request Oct 24, 2017

2017-10-24, Node.js Version 8.8.0 (Current)
Notable Changes:

* crypto:
  - expose ECDH class
    #8188
* http2:
  - http2 is now exposed by defualt without the need for a flag
    #15685
  - a new environment varible NODE\_NO\_HTTP2 has been added to allow
    userland http2 to be required
    #15685
  - support has been added for generic `Duplex` streams
    #16269
* module:
  - resolve and instantiate loader pipeline hooks have been added to
    the ESM lifecycle
    #15445
* zlib:
  - CVE-2017-14919 - In zlib v1.2.9, a change was made that causes an
    error to be raised when a raw deflate stream is initialized with
    windowBits set to 8. On some versions this crashes Node and you
    cannot recover from it, while on some versions it throws an
    exception. Node.js will now gracefully set windowBits to 9
    replicating the legacy behavior to avoid a DOS vector.
    nodejs-private/node-private#95

PR-URL: nodejs-private/node-private#98

@teppeis teppeis referenced this pull request Oct 24, 2017

Closed

doc: remove loader hooks from unsupported features #16465

2 of 4 tasks complete

addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017

2017-10-24, Node.js Version 8.8.0 (Current)
Notable Changes:

* crypto:
  - expose ECDH class
    nodejs/node#8188
* http2:
  - http2 is now exposed by defualt without the need for a flag
    nodejs/node#15685
  - a new environment varible NODE\_NO\_HTTP2 has been added to allow
    userland http2 to be required
    nodejs/node#15685
  - support has been added for generic `Duplex` streams
    nodejs/node#16269
* module:
  - resolve and instantiate loader pipeline hooks have been added to
    the ESM lifecycle
    nodejs/node#15445
* zlib:
  - CVE-2017-14919 - In zlib v1.2.9, a change was made that causes an
    error to be raised when a raw deflate stream is initialized with
    windowBits set to 8. On some versions this crashes Node and you
    cannot recover from it, while on some versions it throws an
    exception. Node.js will now gracefully set windowBits to 9
    replicating the legacy behavior to avoid a DOS vector.
    nodejs-private/node-private#95

PR-URL: nodejs-private/node-private#98

@guybedford guybedford referenced this pull request Nov 12, 2017

Closed

loader: refactor loader #16874

3 of 3 tasks complete

targos added a commit that referenced this pull request Nov 23, 2017

doc: document resolve hook formats
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: #16375
Refs: #15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

2017-10-24, Node.js Version 8.8.0 (Current)
Notable Changes:

* crypto:
  - expose ECDH class
    nodejs/node#8188
* http2:
  - http2 is now exposed by defualt without the need for a flag
    nodejs/node#15685
  - a new environment varible NODE\_NO\_HTTP2 has been added to allow
    userland http2 to be required
    nodejs/node#15685
  - support has been added for generic `Duplex` streams
    nodejs/node#16269
* module:
  - resolve and instantiate loader pipeline hooks have been added to
    the ESM lifecycle
    nodejs/node#15445
* zlib:
  - CVE-2017-14919 - In zlib v1.2.9, a change was made that causes an
    error to be raised when a raw deflate stream is initialized with
    windowBits set to 8. On some versions this crashes Node and you
    cannot recover from it, while on some versions it throws an
    exception. Node.js will now gracefully set windowBits to 9
    replicating the legacy behavior to avoid a DOS vector.
    nodejs-private/node-private#95

PR-URL: nodejs-private/node-private#98

MylesBorins added a commit that referenced this pull request Dec 12, 2017

doc: document resolve hook formats
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: #16375
Refs: #15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

MylesBorins added a commit that referenced this pull request Dec 12, 2017

doc: document resolve hook formats
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: #16375
Refs: #15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

gibfahn added a commit that referenced this pull request Dec 19, 2017

doc: document resolve hook formats
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: #16375
Refs: #15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

gibfahn added a commit that referenced this pull request Dec 20, 2017

doc: document resolve hook formats
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: #16375
Refs: #15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

doc: document resolve hook formats
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: nodejs#16375
Refs: nodejs#15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

doc: document resolve hook formats
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: nodejs#16375
Refs: nodejs#15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment