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(template-vite): switch to esm by default. #3572

Closed

Conversation

caoxiemeihao
Copy link
Member

@caoxiemeihao caoxiemeihao commented Apr 20, 2024

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

Closes #3502, #3503

@caoxiemeihao caoxiemeihao requested a review from a team as a code owner April 20, 2024 06:19
@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Apr 20, 2024

Currently it is not possible to directly change the module in tsconfig.json to ESNext because it will cause an error in forge.config.cts when running yarn start.

image

🚧 But this actually does not affect the running and building of vite template project, because Vite does not care about tsconfig.json.

  • src/main.ts
    image

  • tsconfig.json
    image

@sparecycles
Copy link

This looks like it's less like "support esm", and more like "switch to esm."

Not a bad thing really, but is this another potentially breaking change that's going to land in another minor release? 🤔

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Apr 22, 2024

This looks like it's less like "support esm", and more like "switch to esm."

Not a bad thing really, but is this another potentially breaking change that's going to land in another minor release? 🤔

v7.3.0 can be implemented through user modification to support ESModule, which is not constrained by @electron-forge/plugin-vite.

#3572 is just making some changes to the template, not a major upgrade.

If you have any concerns about this, I can consider closing this PR and only providing a document to inform users how to switch to esm

I think when the vite-template is built using the esm format by default, some users may step up and ask why it does not support cjs. Always stand up and ask questions instead of looking for their own problems.


I am initiating a vote below, hoping that people who default to using the esm or cjs format can click on the corresponding 👍

@caoxiemeihao

This comment was marked as outdated.

@caoxiemeihao

This comment was marked as outdated.

@caoxiemeihao caoxiemeihao changed the title feat(template-vite): support ESModule feat(template-vite): switch to esm by default. Apr 22, 2024
@sparecycles
Copy link

I kind of liked your idea of building according to the package.json's .module property.

I don't know if it's a good idea to separate the ideas of

  • build using esm (build system runtime) and
  • build into esm (electron runtime... and then further configuring for the host, the preload build, and the renderer).

I'm seeing, for instance, with this PR that the preload script is still being built into commonjs (and two commented links adding some context for that), while the host and renderer are defaulting to "follow the package.json".

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Apr 22, 2024

For the file suffixes and build formats of Preload script. I also feel very confused, I have been studying here for a long time.

https://github.com/electron-vite/vite-plugin-electron/blob/v0.28.6/src/simple.ts#L54-L82

image

@sparecycles
Copy link

sparecycles commented Apr 22, 2024

@caoxiemeihao in local testing in my single environment, I found that with package.json type=module, I needed to not only compile preload into cjs but also specify the extension:

new BrowserWindow({
    webPreferences: {
      preload: resolveRelative("./preload.cjs"),
    },
  })

Otherwise node would complain about the importing a module with require (regardless of the content of preload being commonjs or esmodule).

Testing with webPreferences: { contextIsolation: true, preload: resolveRelative("./preload.mjs") }

image

I'm not finding a path to import an esm-compiled preload script at all... (skill issue?)

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Apr 25, 2024

I've provided samples for esm.

@sparecycles
Copy link

If you try adding in the above sample preload.js files:

import * as e from 'electron';

and check the compiled output, they are commonjs (as specified by the cjs format).

They load fine, as they're not esm, and I checked and it doesn't matter if I use a .mjs extension or not.

(If you set the format to es then electron fails to load the preload script.)

@IsaiahByDayah
Copy link

I've provided samples for esm.

For folks finding this who may be using typescript + tailwind and still having issues, make note of a change to the preload extension when creating BrowserWindows as well as this note about the extension on your postcss config

Note that postcss config files do not support ESM + TypeScript (.mts or .ts in "type": "module") yet. If you have postcss configs with .ts and added "type": "module" to package.json, you'll also need to rename the postcss config to use .cts.

After going through and updating my files to match forge-esm-ts I think things are running fine 🙏🏾

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.

3 participants