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

fix: expose the built-in electron module via the ESM loader #35930

Merged
merged 1 commit into from Oct 10, 2022

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Oct 6, 2022

What is this?

This specifically enables the following two patterns:

// foo.mjs
import { app } from 'electron';
// foo.js or foo.mjs
async function main() {
  const { app } = await import('electron');
}

What isn't this?

This is not "ESM support for Electron". I classify that as supporting an ESM entrypoint for an Electron app. Currently for reasons well outlined in the requisite issue #21457 an Electron app must have a CJS entrypoint. It can then immediately farm out to an ESModule but it's important apps understand the limitations / issues with that approach, hence we do not allow an mjs file to be directly used as an entrypoint.

How does it work?

Weirdly, this is like the 4th time I've tried this. And this time I found a relatively lightweight patch to make it work, basically we trick the ESM module resolver in node.js to consider "electron" the url electron:electron which then routes to the commonjs esm loader and loads the electron module straight out of our Module._cache entry we inject it into ✨

As a bonus this PR comes with tests proving it works 😄

I can claim this as semver/patch given it was always kinda supposed to work (it's a bug that you can load a module via require and not via import) so I think this is a fine backport candidate.

Notes: You can now import the built-in electron module via ESModule loaders, i.e. import('electron') and import 'electron' now work natively

@MarshallOfSound MarshallOfSound added semver/patch backwards-compatible bug fixes target/20-x-y target/21-x-y PR should also be added to the "21-x-y" branch. target/22-x-y PR should also be added to the "22-x-y" branch. labels Oct 6, 2022
@MarshallOfSound MarshallOfSound requested a review from a team as a code owner October 6, 2022 11:17
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 6, 2022
@MarshallOfSound MarshallOfSound mentioned this pull request Oct 6, 2022
3 tasks
@inukshuk
Copy link
Contributor

inukshuk commented Oct 6, 2022

Wow, that's great! I assume this will only work using the Node.js ESM module loader and not in a Renderer with Node Integration?

@MarshallOfSound
Copy link
Member Author

Haven't tested, but I believe that to be correct. V8 allows a single ESM loader per isolate, not per context. Which means in the renderer we have to choose between nodes ESM loader and blinks. Obviously we're going to choose blinks to ensure the web, well, works 😅

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 7, 2022
@MarshallOfSound MarshallOfSound merged commit 1fe21ff into main Oct 10, 2022
@MarshallOfSound MarshallOfSound deleted the esm-loader-support branch October 10, 2022 10:02
@release-clerk
Copy link

release-clerk bot commented Oct 10, 2022

Release Notes Persisted

You can now import the built-in electron module via ESModule loaders, i.e. import('electron') and import 'electron' now work natively

@trop
Copy link
Contributor

trop bot commented Oct 10, 2022

I have automatically backported this PR to "20-x-y", please check out #35956

@trop
Copy link
Contributor

trop bot commented Oct 10, 2022

I have automatically backported this PR to "22-x-y", please check out #35957

@trop
Copy link
Contributor

trop bot commented Oct 10, 2022

I have automatically backported this PR to "21-x-y", please check out #35958

@trop trop bot added in-flight/22-x-y merged/22-x-y PR was merged to the "22-x-y" branch. and removed target/22-x-y PR should also be added to the "22-x-y" branch. target/21-x-y PR should also be added to the "21-x-y" branch. in-flight/22-x-y labels Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/21-x-y PR was merged to the "21-x-y" branch. merged/22-x-y PR was merged to the "22-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants