-
-
Notifications
You must be signed in to change notification settings - Fork 602
feat(plugin-vite): automatically switch to esm mode when enabled #4016
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
base: main
Are you sure you want to change the base?
Conversation
87cce28 to
35082c7
Compare
BlackHole1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL @caoxiemeihao
|
Could we move on this? Are there any blockers? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this file? Will the absence of this file cause test failures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I could make it optional but IIRC there is some other code that actually relies on there always being a package.json anyway, except that it's not hit by the tests so the fixture worked before
Signed-off-by: maximsmol <[email protected]>
Signed-off-by: maximsmol <[email protected]>
Signed-off-by: maximsmol <[email protected]>
11adf77 to
19e674c
Compare
erickzhao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @maximsmol, thanks for the PR. I recall having some offline discussion about this, and will get back with a review soon.
Please note that you have to get your commits signed for any contributions to Electron!
Summarize your changes:
Electron now supports ESM by settings
"type": "module"inpackage.jsonbut@electron-forge/plugin-vitewill output CommonJS code regardless, which will then fail to load (typically by trying to referencerequire).This patch reads
typefrompackage.jsonand automatically changes the main vite config output format toesmif modules are requested. Note that we parsepackage.jsontwice now (the first time to look for a forge config) but this seemed cleaner than trying to pass the parsed JSON around.I am deliberately leaving preload scripts as-is since they ignore
"type": "module"and load fine with the current settings. That being said, Electron supports ESM preload scripts, so we might want to automatically change the output format there as well. This would also require renaming the output from.jsto.mjsThere is also the sandboxing limitation, which may mean that bundling to CommonJS is the best way for preload scripts regardless, but I do not understand well enough how people use preload scripts (and in particular how common sandboxing is) to make that decision.Please let me know if I need to add docs or automated tests here—the change is pretty trivial so I did not bother.
Related: #3684