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

feat: Node compatible ESM build #756

Merged
merged 5 commits into from
Oct 2, 2023
Merged

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Sep 29, 2023

This PR changes to using Rollup to build the output files.

This should fix main process ESM compatibility (#751) but since Electron v28 is still published under electron-nightly there is no easy way to add this to our test matrix until they publish a beta release.

I have tested this locally with a nightly build but I don't think it's worth the effort or adding nightly support to our e2e test suite when there will likely be a beta built in a few weeks.

  • Because Rollup requires the TypeScript module target to be ESNext and then Rollup converts to CJS, this caused a few incompatibility issues with existing side effects.
    • TypeScript enums are evil but we can't change them until the next major
    • Some side-effects appear unavoidable to keep abnormal session tracking working correctly
  • Detection of code loaded in the incorrect process relied on require order which isn't compatible with ESM
  • Now we're using Rollup we can bundle the preload scripts simply
  • Preload injection required some changes to work without require.resolve

@timfish timfish marked this pull request as ready for review September 29, 2023 12:37
@AbhiPrasad
Copy link
Member

Gonna review this on Monday - having the bundled preload script is really nice.

@timfish
Copy link
Collaborator Author

timfish commented Sep 29, 2023

having the bundled preload script is really nice

No more weird manual bundle creation!

@timfish

This comment was marked as outdated.

Comment on lines +19 to +26
? require.resolve('../../preload/legacy.js')
: require.resolve('../../preload/index.js');
} catch (_) {
try {
// This could be ESM
const currentDir = fileURLToPath(import.meta.url);
// Use the CJS preload
return resolve(currentDir, '..', '..', '..', '..', 'preload', 'index.js');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this will break with any bundlers, but if the e2e tests pass we should be fine I guess.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preload injection mostly fails anyway with bundlers anyway and we fallback to custom protocol.

Once Electron v28 makes it to beta we can add some ESM main process bundler tests.

app.once('ready', () => {
resolve();
if (app) {
return app.isReady()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when is app not defined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this code gets loaded into the Electron renderer rather than the main process.

We have code that displays a warning if you load the wrong code into the wrong process. Rollup requires tha we target module: esnext and it then transpiles to CJS/ESM. The issue with this is that imports/side-effects are changed/moved and these bits of code get hit before our warning code!

@timfish timfish self-assigned this Oct 2, 2023
@timfish timfish merged commit e52864a into getsentry:master Oct 2, 2023
41 checks passed
@timfish timfish deleted the feat/esm branch October 2, 2023 14:50
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

Successfully merging this pull request may close these issues.

2 participants