Contexts: supporting new Node's ESM Loader (without hacking) #33

SMotaal · 2018-03-30T17:54:13Z

Version: Electron 2.0.0-beta.x
Platform: macOS 10.13
Subsystem: electron/node

After over a month of testing all aspects of Node's new loader infrastructure in electron, only one issue in my opinion remains to be resolved and in my best estimation it needs to be in electron/node.

Setup: (steps to use Node's loader)

// Construct
const Loader = process.NativeModule.require('internal/loader/Loader');
const loader = new Loader(`file://${__dirname}`);

// Configure (abstract)
loader.hook({
  resolve(specifier, referrer, resolve) {
    // Custom resolution logic …
    // return {url: ..., format: ... }

    // Or defer to default resolver
    return resolve(specifier, referrer);
  }
});

// Import a module
loader.import('./module.js', `file://${__filename}`);

Problem

// Import a module (works the first time)
loader.import('./module.js', `file://${__filename}`);

// Re-Importing a module fails *** throws ***
loader.import('./module.js', `file://${__filename}`);

The loader's internals are not designed to work in multi-context environments. Both Electron and NW.js present the same issue where the loader associates the "evaluated" namespace of the module to the current node context which seems to not behave the same here as it does in node.

The end result is that a module will load fine on first import where the loader holds a direct reference to it, but as soon as it is imported again (if even on the next line) then the loader uses the stored reference in the module map expecting it to point to the context-bound instance of the module.

Unlike Node, in both Electron & NW.js, this instance would be undefined.

Workaround

It takes two ugly but effective steps (all kosher and pure JS). 1. Override the loader's import method so that it keeps a reference of the first instance of the module on the actual JS instance of the module wrap (ie moduleWrap[Symbol.for('module')]). 2. At the same time, make sure to always create a separate loader instance per context (in Electron, I create one for main, and one in preload which is reliable both in preload as well as the associated window).

Recommendation

How Electron or NW.js can best go about resolving this depends on their multi-context architectures, and best left to those more knowledgeable. However, from all my research and testing, I am almost certain that correctly associating contexts with modules is somewhere in src/module_wrap.cc.

I believe that Node's loader will take little effort to make it ideal for mainstream use in Electron. If you are looking into it, I am very interested and would be happy to exchange recipes regarding other related obstacles. As it stands, the loader is very stable if you know what to expect, and it takes a lot of patching. I managed to do it all in JS and without any custom rebuilds. If this will eventually become mainstream, I honestly think some effort will be needed to properly support it by the platform.

The text was updated successfully, but these errors were encountered:

MylesBorins · 2018-03-30T17:57:50Z

Nodes esm loader is still very much in flux and may change significantly before stabilizing. Just want to give a heads up

…

On Fri, Mar 30, 2018, 1:54 PM Saleh Abdel Motaal ***@***.***> wrote: - *Version*: Electron 2.0.0-beta.x - *Platform*: macOS 10.13 - *Subsystem*: electron/node After over a month of testing all aspects of Node's new loader infrastructure in electron, only one issue in my opinion remains to be resolved and in my best estimation it needs to be in electron/node. *Setup*: (steps to use Node's loader) // Construct const Loader = process.NativeModule.require('internal/loader/Loader'); const loader = new Loader(`file://${__dirname}`); // Configure (abstract) loader.hook({ resolve(specifier, referrer, resolve) { // Custom resolution logic … // return {url: ..., format: ... } // Or defer to default resolver return resolve(specifier, referrer); } }); // Import a module loader.import('./module.js', `file://${__filename}`); *Problem* // Import a module (works the first time) loader.import('./module.js', `file://${__filename}`); // Re-Importing a module fails *** throws *** loader.import('./module.js', `file://${__filename}`); The loader's internals are not designed to work in multi-context environments. Both Electron and NW.js present the same issue where the loader associates the "evaluated" namespace of the module to the current node context which seems to not behave the same here as it does in node. The end result is that a module will load fine on first import where the loader holds a direct reference to it, but as soon as it is imported again (if even on the next line) then the loader uses the stored reference in the module map expecting it to point to the context-bound instance of the module. Unlike Node, in both Electron & NW.js, this instance would be undefined. *Workaround* It takes two ugly but effective steps (all kosher and pure JS). 1. Override the loader's import method so that it keeps a reference of the first instance of the module on the instance of the module wrap. 2. At the same time, make sure to always create a separate loader instance per context (in Electron, I create one for main, and one in preload which is reliable both in preload as well as the associated window). *Recommendation* How Electron or NW.js can best go about resolving this depends on their multi-context architectures, and best left to those more knowledgeable. However, from all my research and testing, I am almost certain that correctly associating contexts with modules is somewhere in src/module_wrap.cc. I believe that Node's loader will a little more effort to make it ideal in Electron. If you are looking into it, I am very interested and would be happy to exchange recipes regarding other related obstacles. As it stands, the loader is very stable if you know what to expect, and it takes a lot of patching. I managed to do it all in JS and without any custom rebuilds. If this will eventually become mainstream, I honestly think some effort will be needed to properly support it by the platform. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#33>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV9Qco40iRt6xLLcVDkAIcTBf76ntks5tjnFGgaJpZM4TB9eI> .

SMotaal · 2018-03-30T18:16:02Z

That is very true, many aspects are in flux, especially with the more recent additions like dynamic imports and import.meta (other issues like what specifiers or packages ought to be or not are more of a backdrop from my perspective until the community works those out).

As it stands, Electron 2.0.0-beta.x deals only deals with the more stable aspects of the loader, ie static imports (Node 8.x and Chrome 61). Would it not be a safe bet to assume that the implementation is rather stable with that regard.

zcbenz · 2018-04-02T04:00:06Z

It is surprising to me that it does not work, in Electron the Node environment is created on the context of web pages, so normally we should have Node code and user code run in the same context. I'll need to look into the internals of the new loader to know better.

MylesBorins · 2018-04-02T04:15:20Z

/cc @bmeck who can chime in on the internals

bmeck · 2018-04-02T13:21:33Z

I know that Chrome ties their loader Module Map to Context data using Context::GetEmbedderData, and that V8 has warned us to not have the C++ resolution resolve to a Module from another context, but I don't think any internals are tied to a context by Node's loader. I don't know electron's setup well enough, but I do know that moduleWrap.* is never intended to be run in a different context from where it was allocated, but I cannot see how it could do that unless someone is pulling it out of Node's internals. I am wondering if the idempotentcy rules implemented by V8 might be involved here as well: https://github.com/v8/v8/blob/df35adc763d6da0c517702c3d38dec86dcff75cb/src/objects/module.cc#L317 .

That said, I would heavily encourage not using it while it is still flagged and subject to change.

devsnek · 2018-04-02T14:04:00Z

fwiw i can't reproduce any context-related crashing/errors with ModuleWrap. as i know it the only thing that should fail is dynamic import which we currently lock to the main context. +1 to not using node's loader at all for the moment. @SMotaal if you could let me know what the specific error you got is that would be pretty helpful :)

SMotaal · 2018-04-03T00:37:16Z

@devsnek I created a quick repo to help illustrate this (Electron-specific) issue:
https://github.com/code-therapy/electron-module-wrap-issues

The same thing also happens in NW.Js (maybe for different reasons) but I am not able to repo that at this point.

The demo simply creates a loader in each context (main, preload and window) then imports the same module twice (with fallback to append a hash if the attempt fails).

Here is a quick output from stdout:

Main

  [Main]

        Import: 1
        Attempt:        1
        Specifier:      "file://./test.mjs"

   { default: 'Magic number: 3' }

  [Main]

        Import: 2
        Attempt:        1
        Specifier:      "file://./test.mjs"

   TypeError: Cannot read property 'evaluate' of undefined
      at ModuleJob.run (internal/loader/ModuleJob.js:94:14)
      at <anonymous>

  [Main]

        Import: 2
        Attempt:        2
        Specifier:      "file://./test.mjs#2"

   { default: 'Magic number: 6' }

Here is the same from the console (preload part):

[Preload]

	Import:	2
	Attempt:	1
	Specifier:	"file://./test.mjs"

 TypeError: Cannot read property 'evaluate' of undefined
    at ModuleJob.run (internal/loader/ModuleJob.js:94)
    at <anonymous>
    testImport @ ./main.js:53
    ./main.js:47 

[Preload]

	Import:	2
	Attempt:	2
	Specifier:	"file://./test.mjs#2"

 Module

Here is the same from the console (window part):

[Window]

	Import:	1
	Attempt:	1
	Specifier:	"file://./test.mjs"

 Module

[Window]

	Import:	2
	Attempt:	1
	Specifier:	"file://./test.mjs"

 TypeError: Cannot read property 'evaluate' of undefined
    at ModuleJob.run (internal/loader/ModuleJob.js:94)
    at <anonymous>
    testImport @ main.js:53
    main.js:47 
		
[Window]

	Import:	2
	Attempt:	2
	Specifier:	"file://./test.mjs#2"

 Module

devsnek · 2018-04-03T04:06:45Z

@SMotaal i believe this issue was inadvertently already fixed. in ModuleJob#instantiate the this value would be lost in certain cases i don't anyone completely understands resulting in it returning undefined so down in ModuleJob#run module.evaluate() would throw. this behavior should not occur on the master branch, although i assume you can't work with the master branch in electron so probably not possible to confirm this is fixed.

SMotaal · 2018-04-03T11:24:26Z

@devsnek, well, with my understanding of the standards, I make the assumption that the module evaluates once so I keep the evaluated/instantiated module promise from the first time and use it in subsequent calls to loader.import but this only works for static imports (till Chrome 63).

Dynamic imports down the road will not work with this (js side) fix because the context can/will be destroyed on reload while the loader is still hooked to isolate's callback, which makes it very tricky to manage especially if some other reload surprise kicks in at the same time. So I was hoping this would be stable on a ModuleWrap level (excuse my ignorance of the upstream but trust me I know ModuleWrap is not the bad guy here, thanks ModuleWrap) so that graceful loader replacement can be manageable on reload or avoidable all together (that would be super).

SMotaal · 2018-04-03T11:42:25Z

@devsnek I should also not one crazy thing I noticed concurrently when this error happens, if I used setTimeout to call the testLoader [ie await > setTimeout > [Preload] testLoader() > then > await > setTimeout > [Window] testLoader()] it all fires instantly (likely in correct sync order) so that window sometimes resolves faster than preload and more importantly without honouring the timeout aspect (regardless of the value I set).

pauliusuza · 2018-06-27T12:45:33Z

Now that electron 3.0.0-beta.1 is out, has anyone tried doing static imports from the client html modules. Is having import syntax across the whole app achievable at this stage?

devsnek · 2018-06-27T16:48:19Z

the renderer should use chromium's loader right? this seems like a really confusing situation.

pauliusuza · 2018-06-27T19:49:27Z

Yeah. I think frankly this should be a flag, something you pass into the webPreferences like use_esm_loader: true and it would then use node.js back-end loader inside the html and any client side modules.

SMotaal · 2018-06-30T13:17:12Z

@pauliusuza Sorry, I needed time to update my blobbing abstraction to catch up with all the changes that happened between electron and node internals. Now that my experimental app is up and running, I can start to experiment.

I am open for running some experiments between chromium's and node's loaders so please feel free to suggest scenarios. Fenced code blocks of simplified exporting/importing modules can really help minimize ambiguity — preferred over the actual modules because all I am testing is resolution paths and ES module bindings.

A sample scenario is:

import electron, { app } from 'electron'

Considerations

Browsers do not support bare specifiers
Node's CJS translator exports "default" (not named exports)
'electron' is a CommonJS module
'electron' is loaded from ASAR bundle

Modules

//tests/modules/exports-electron-default

// Chromium expectation: INVALID SPECIFIER
// Node expectation: PASS

// Test scopes: main, preload, window

export {default as exported} from 'electron'

//tests/modules/exports-electron-app

// Chromium expectation: INVALID SPECIFIER
// Node expectation: INVALID BINDING

// Test scopes: main, preload, window

export {app as exported} from 'electron'

ken-okabe · 2018-08-11T01:44:52Z

I'm following this issue, and in terms of the design concept of Electron, I also had the same impression of @zcbenz.

Here's my thought.

Node module eco system (npm) has been attractive, and that has been one of the major reason to use Node, and now ESM has become defacto-standard and all of the modern browsers support it including mobile browsers.

Node has struggled to catch up to this new standard mostly because the own foundation is NPM, another module eco, hence, the topic is fundamentally an issue between 2 different module ecosystems .

This issue has been and will be prolonged for a while because now, for some reason, ECMAScript evolves so rapidly, ecma2015, 2017, 2018, and all modern browser are eagerly catching up, and evolves fast, but Node is not.

As a developer, I would like to catch up the latest standard, and the browsers mostly satisfy my/our demand, and in this sense, Node becomes much more problematic recently. So, I probably need to make a decision, not to use Node as much as possible, but browsers engine as much as possible. In any scenario, mixing up 2 different module ecosystems is not a healthy situation. So, what I am trying to do is to convert NPM libraries that I use to ESM, for instance, React16.* has not released ESM version, but probably in ver17. I obtained ESM version by my self. Probably, most certainly in near future, some projects to aim to convert CJS(npm libraries)->ESM will emerge in JavaScript community.

So, probably the workaround would be to launch a small server inside Electron with a pure Chromium client script. In this way, all of the node ESM glitches is resolved. Perhaps, a hook can be prepared to convert the desired NPM to ESM to a www/esm directory that the client script will import from ./esm/foo.js

In this workaround, why do we use Electron, not just the browsers? To me, the browser still has many restrictions to access local files, watch the file change and reload the contents etc.

jdalton · 2018-08-11T02:28:08Z

In this workaround, why do we use Electron, not just the browsers? To me, the browser still has many restrictions to access local files, watch the file change and reload the contents etc.

In the meantime there's also loaders, like esm, that can enable a consistent ESM experience in the main and renderer processes without much fuss.

ken-okabe · 2018-08-11T05:49:52Z

@jdalton
Ok, that's a beautifully implemented bridge, and clearly the search-engine or my-head didn't work well, so I must withdraw the previous remarks : )

In fact, your library makes me refactor my on-going project, and I hope your implementation to Electron should be the default pattern, at least, a major alternative one. It seems you are now in position to be able to commit Electron project in deep. So I think that's also fortunate to all of us.

Thanks a lot.

SMotaal · 2018-08-11T11:39:46Z

@kenokabe I've been through this, tried to refactor existing work as things progress. I came to the realization that everything is a moving target, so best to keep current work isolated and experiment meanwhile with new patterns in experimental playgrounds of my own design. Just like you, I need to understand new patterns and really find it very counter-productive to continue working on my on going projects knowing they are implemented in retro. In my case, circumstances forced my hand into too much free time to experiment and I welcomed it, so did my collaborators on the projects I cared about and wanted to keep working on.

My latest focus has been on browsers, not all browsers, just the ones that play nice, the ones that are likely to be PWA or App targets in the next few years, and Mozilla (cause they are just awesome folks). Here is a quick proof-of-concept of what it takes to reimagine bundling that retains ESM (and all other) files without any of the old-school bundling which relied on non-ESM module constructs. It is too early to present this better at the moment and the demo is strictly a browser thing, but my motivation is to devise ways to cleanly create projects without too many devDependency boilerplate and forced configs as with create-that-app stuff that has gotten way too ugly for anyone with a brain to be willing to use in their projects.

Edit: I am gearing my experimental prototype project for Electron/Chrome assuming that other platforms will evolve, but not too concerned if they don't, it's about potential not adoption.

ken-okabe · 2018-08-12T01:43:50Z

@SMotaal Right, after Firefox Quantum release, I switched my main browser to Firefox and thought it's good to have both Firefox/Chromium in the world. In terms of Service-Worker, I expect adaptations by Gmail or GoogleMap will be a tipping point, like the AJAX shock to JavaScript
community delivered by GoogleMap, so will see.

By the way, for this issue, I somewhat have an impression that the outstanding technology provided by @jdalton and his implementation pattern to Electron : https://github.com/standard-things/electron-quick-start pretty much have resolved this issue; to use esm instead of Node's ESM Loader. I'm not 100% sure of your purpose, but to me it does the job. What do you think.

SMotaal · 2018-08-12T13:31:39Z

@kenokabe absolutely, I was trying to imply using @jdalton's pattern not the --experimental-modules, ie to not refactor working code for experimental conditions and instead to carry out your experiments as side projects. The extra bit was about one such experiment I happen to be working on.

HughxDev · 2018-09-14T11:13:06Z

In the meantime there's also loaders, like esm, that can enable a consistent ESM experience in the main and renderer processes without much fuss.

@jdalton Thank you very much for this library, however I’m not able to use it in the renderer because it violates CSP without unsafe-eval set, which is a security vulnerability.

jdalton · 2018-09-14T15:17:03Z

Hi @hguiney!

Let's take care of that over on the issue tracker. I have a hunch at the root cause. I'll fix that right up.

Update:

Fixed in v3.0.83 🎉

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contexts: supporting new Node's ESM Loader (without hacking) #33

Contexts: supporting new Node's ESM Loader (without hacking) #33

SMotaal commented Mar 30, 2018 •

edited

Loading

MylesBorins commented Mar 30, 2018 via email

SMotaal commented Mar 30, 2018 •

edited

Loading

zcbenz commented Apr 2, 2018

MylesBorins commented Apr 2, 2018

bmeck commented Apr 2, 2018

devsnek commented Apr 2, 2018 •

edited

Loading

SMotaal commented Apr 3, 2018 •

edited

Loading

devsnek commented Apr 3, 2018

SMotaal commented Apr 3, 2018

SMotaal commented Apr 3, 2018

pauliusuza commented Jun 27, 2018

devsnek commented Jun 27, 2018

pauliusuza commented Jun 27, 2018

SMotaal commented Jun 30, 2018

ken-okabe commented Aug 11, 2018 •

edited

Loading

jdalton commented Aug 11, 2018

ken-okabe commented Aug 11, 2018

SMotaal commented Aug 11, 2018 •

edited

Loading

ken-okabe commented Aug 12, 2018

SMotaal commented Aug 12, 2018

HughxDev commented Sep 14, 2018

jdalton commented Sep 14, 2018 •

edited

Loading

Contexts: supporting new Node's ESM Loader (without hacking) #33

Contexts: supporting new Node's ESM Loader (without hacking) #33

Comments

SMotaal commented Mar 30, 2018 • edited Loading

MylesBorins commented Mar 30, 2018 via email

SMotaal commented Mar 30, 2018 • edited Loading

zcbenz commented Apr 2, 2018

MylesBorins commented Apr 2, 2018

bmeck commented Apr 2, 2018

devsnek commented Apr 2, 2018 • edited Loading

SMotaal commented Apr 3, 2018 • edited Loading

devsnek commented Apr 3, 2018

SMotaal commented Apr 3, 2018

SMotaal commented Apr 3, 2018

pauliusuza commented Jun 27, 2018

devsnek commented Jun 27, 2018

pauliusuza commented Jun 27, 2018

SMotaal commented Jun 30, 2018

ken-okabe commented Aug 11, 2018 • edited Loading

jdalton commented Aug 11, 2018

ken-okabe commented Aug 11, 2018

SMotaal commented Aug 11, 2018 • edited Loading

ken-okabe commented Aug 12, 2018

SMotaal commented Aug 12, 2018

HughxDev commented Sep 14, 2018

jdalton commented Sep 14, 2018 • edited Loading

SMotaal commented Mar 30, 2018 •

edited

Loading

SMotaal commented Mar 30, 2018 •

edited

Loading

devsnek commented Apr 2, 2018 •

edited

Loading

SMotaal commented Apr 3, 2018 •

edited

Loading

ken-okabe commented Aug 11, 2018 •

edited

Loading

SMotaal commented Aug 11, 2018 •

edited

Loading

jdalton commented Sep 14, 2018 •

edited

Loading