Skip to content
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

[Feature Request]: Migrate to using native ESM modules instead of CommonJS #3129

Closed
3 tasks done
martinszeltins opened this issue Jan 4, 2023 · 9 comments
Closed
3 tasks done

Comments

@martinszeltins
Copy link

Pre-flight checklist

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project uses.
  • I have searched the issue tracker for a feature request that matches the one I want to file, without success.

Problem description

Right now when you create a new Electron app using Forge npm init electron-app@latest my-app it still uses the old CommonJS require statements. Node has had native ESM module support for a long time and it just makes sense to switch to it so it is more modern and the Javascript community has moved to it.

Current app genereted with Forge:

const { app, BrowserWindow } = require('electron');
const path = require('path');
...

Proposed solution

Use modern native ESM modules instead:

import { app, BrowserWindow } from 'electron'
import path from 'path'

Alternatives considered

There are no alternatives.

Additional information

No response

@MarshallOfSound
Copy link
Member

Per the upstream Electron issue about using ESM in apps --> electron/electron#21457 (comment)

This isn't something that I would suggest people use let alone suggest we set it as a default. It's full of footguns even if you can make it work. Our guidance has always been and will remain:

If you want to use import and ESM, transpile it back to commonjs either via bundling (webpack, vite, etc.) or via an in-place transform (tsc, babel)

@MarshallOfSound MarshallOfSound closed this as not planned Won't fix, can't repro, duplicate, stale Jan 4, 2023
@MarshallOfSound
Copy link
Member

Worth noting if you generate a forge app using the webpack typescript template you get import / ESM syntax in the generated app too

@paulmillr
Copy link

@MarshallOfSound

Our guidance has always been and will remain

Why? ESM is natively supported in browsers. You deliberately turn off the native feature?

There are no commonjs modules in browsers.

@martinszeltins
Copy link
Author

martinszeltins commented Feb 6, 2023

@MarshallOfSound

Our guidance has always been and will remain

Why? ESM is natively supported in browsers. You deliberately turn off the native feature?

There are no commonjs modules in browsers.

I think the reason for not supporting ESM is because it would be a MASSIVE undertaking to refactor the current electron / node codebase to support it. It would almost be easier to create electron from scratch than to support ESM modules in the current codebase.

@paulmillr
Copy link

MASSIVE undertaking to refactor the current electron / node codebase to support it

Like, executing "Find and Replace" using regexps?

@erickzhao
Copy link
Member

erickzhao commented Feb 6, 2023

@paulmillr If you manage to find a trivial regexp solution that passes all our tests, PRs are always welcome into the main electron/electron repo. You can get a local build of Electron running using https://github.com/electron/build-tools. :-)

@MarshallOfSound's comments in the linked GitHub issue above describes the issue in more detail. There are more technical implications regarding the async nature of ESM entrypoints that breaks some guarantees from Electron's boot flow. If you want to read through the full text:

electron/electron#21457 (comment)

Hey folks,

Having looked into this somewhat from a technical perspective here is where we currently stand. Supporting ESM as the entrypoint into an Electron app is fundamentally incompatible with how Electron apps work.

Specifically ESM modules are loaded asynchronously including the main entry point, there is no way to load one syncronously.

The current boot flow of an Electron app is as follows:

Electron starts initialization
//content initialization
node_js initialization
{time passes}
//content ready --> electron app.emit('ready')

Notably the ready event / the content layer being "ready" is an asynchronous process (due to the creation or worker threads, helper procesess, etc.), CJS modules are loaded synchronously which allows apps to guarantee to run certain javascript before the ready event is emitted. As a result of ESM modules being loaded asynchronously this is no longer possible, and apps can no longer guarantee to initialize certain things like custom protocols, permission handlers, or custom command line flags before the content layer is initailized.

For different reasons we can't allow ESM modules in preload scripts either, ESM module loaders work at the Isolate level in v8, unfortunately even with context isolation both the isolated context your preload runs in and the main world your website runs in live in the same underlying Isolate. This means that you can only attach one ESM loader into v8, either blinks loader that will make the webpage load ESM modules correctly relative to the website address, or node's loader / a custom loader that loads modules relative to the preload path. Supporting both at the same time is a massive security issue and when we had to choose one we obviously chose to use blinks to avoid the cardinal rule of browsers "Don't break the internet" 😄

I understand this isn't the answer people want to hear, modules all over the npm ecosystem are actively moving towards ESM and the fact that for Reasons ™️ you can't use them natively in Electron's main process is frustrating but instead of just an ever-growing thread of confusion I thought it was best to set the record straight.

For now, the only solutions that don't compromise the event flow of your app are to use ESM in your code and use some kind of compiler / bundler to end up with a CJS file. I know there are some workarounds in this thread of using import() from a CJS entrypoint, please do not do that, you will run into race conditions and we won't be able to help you fix them.

We'll undoubtedly circle back on this every now and again, checking if anything has changed at any level and who knows, maybe I missed something and someone will come up with a really smart idea that makes this possible. Until then, keep using webpack or vite or whatever bundler is your favorite at the moment.

electron/electron#21457 (comment)

Could you elaborate a little bit about what the specific race conditions are?

The exact set of "things that wouldn't work" is longer than this but just off the top of my head:

  • Appending custom Chromium command line flags (E.g. --enable-features or --disable-features) has to be done before that flag is consumed, for the majority of flags that is immediately after we run Node.js in the main process
  • Custom protocols normally rely on registerSchemesAsPrivileged and that API must be called before certain undelayable features in Chromium have been initialized, calling that API must occur synchronously during app initialization in order for it to be guaranteed to work
  • Initializing the crash reporter via crashReporter.start should be done as early as possible so that if things later in the app initialization flow cause hard crashes the reporter has been set up and will catch them. Running crashReporter.start asynchronously can easily result in some crashes being completely missed and dropped into the void.

These are just three gotchas I came up with just now, I'm sure there are many others. Although you may be correct that they don't impact your specific app, or you may simply not be aware in the case of things like the crash reporter Electron can not offer a system with these kinds of unresolvable gotchas especially if we know about them before we build it 😆

In my own opinion, using a bundler is really undesirable and we would face a lot of resistance towards that when our current approach of loading ES modules directly is working in such a clean and reliable (for us) way.

Using a bundler has other advantages like a significant boot-time performance boost on most platforms, especially on windows. Reading large numbers of files off disk is quite slow 😅

import ... from "..."; is synchronous.

No it's not, to the JS consumer it might appear to be. But here is a demo app that proves that it isn't. The control flow of the javascript code is linear, that does not mean each command is executed synchronously in the same tick of the event loop. If you think about how ESM module loads work for http modules in a browser you can realize that it can't possibly be synchronous as the mere act of loading the module requires an async network request.

Example app: https://github.com/MarshallOfSound/esm-async-demo

MDN Docs:

There is no need to use the defer attribute when loading a module script; modules are deferred automatically.

There appears to be a confusion here around "async/await" as a javascript feature and the technical concept of something not being synchronous. To clarify, when I say "ESM is async" I mean, the underlying technical implementation is not synchronous when observed by a third party. In this case, when Electron calls nodes ESM loader, even though internally to node everything appears synchronous and in-order, externally async file reads are occurring, JS is being parsed asynchronously, etc. which means that the external application can't make any synchronous guarantees about when your JS code is finished executing which currently is a side effect Electron fundamentally relies on as I outlined above.

You can of course wait for the asynchronous loading and eval work to finish. This is how await import() works. I'd be shocked if there wasn't a way for Electron internals to wait on the import as well.

Similar to what I just wrote above, there is a different between async/await as a JS feature and "asynchronous work" happening. Electron is fundamentally implemented in C++, not in javascript, we can't "await" an ESM load and then continue. In order to support what folks are asking for here properly we would have to provide some kind of "doneLoading()" API, and then delay the chromium event loop until the app has called doneLoading() from the uv loop. And I'm not sure that is even a technical possibility 😅 Trust me, if it was as easy as just saying await loadModule() I would have done it already 😀

@paulmillr
Copy link

@erickzhao Appreciate the clarification. It was hard to find something in the thread.

@mmm8955405
Copy link

mmm8955405 commented Mar 23, 2023

Per the upstream Electron issue about using ESM in apps --> electron/electron#21457 (comment)

This isn't something that I would suggest people use let alone suggest we set it as a default. It's full of footguns even if you can make it work. Our guidance has always been and will remain:

If you want to use import and ESM, transpile it back to commonjs either via bundling (webpack, vite, etc.) or via an in-place transform (tsc, babel)

No specific documentation available! When using the sveltekit, Forge.config.js cannot be loaded correctly and should not be an es module. If you close this issue, it's best to have an alternative solution

@ferminc
Copy link

ferminc commented Mar 23, 2023

I'm using the webpack-typescript template of electron-forge and I noticed it compiles both the renderer and main .ts code to commonjs, but as far as I know, if I'm not using node integration in the renderer process there should be not problem using ES modules there. Can anyone provide me with the correct configuration to make this happen while using webpack and typescript?

In short, I want to use ES modules in the renderer and commonjs in the main process while using typescript

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants