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): add vite template #3071

Merged
merged 26 commits into from
Feb 25, 2023
Merged

feat(template): add vite template #3071

merged 26 commits into from
Feb 25, 2023

Conversation

caoxiemeihao
Copy link
Member

@caoxiemeihao caoxiemeihao commented Nov 13, 2022

  • 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).

Addresses #2954

Summarize your changes:

Support --template=vite :)

Hi! here. 👋
@erickzhao @VerteDinde
Some additional help is needed here.

  1. not sure if the design of VitePluginConfig makes sense, it references WebpackPluginConfig.
  2. I think more configuration should be left to the user to implement in vite.config.xxx rather than integrated in the template.
  3. @electron-forge/plugin-vite needs to write test code

@malept malept marked this pull request as draft November 14, 2022 01:06
@VerteDinde VerteDinde self-requested a review November 14, 2022 06:03
@erickzhao erickzhao requested a review from a team November 16, 2022 17:10
Copy link
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

Left a few initial comments, but the overall approach looks good to me! Thanks so much for doing this work! 🙇‍♀️

To answer #2:

I think more configuration should be left to the user to implement in vite.config.xxx rather than integrated in the template.

I'm fine with this approach, especially for more experienced Vite users, it will allow them more customization options. We can always make the template more or less opinionated in later drafts.

packages/plugin/vite/src/VitePlugin.ts Outdated Show resolved Hide resolved
packages/template/vite/tmpl/renderer.js Show resolved Hide resolved
Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

not sure if the design of VitePluginConfig makes sense, it references WebpackPluginConfig.

In my opinion, the WebpackPluginConfig design could use some improvement. For example, why are config.mainConfig and config.renderer.config named that way? It's not very intuitive.

I think more configuration should be left to the user to implement in vite.config.xxx rather than integrated in the template.

I think this is a good initial approach. We can always add more Forge-specific configuration if needed.

packages/template/vite/tmpl/forge.config.js Show resolved Hide resolved
packages/plugin/vite/src/VitePlugin.ts Outdated Show resolved Hide resolved
@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Nov 17, 2022

Here! 👋
I provided a temporary running solution, it will be a good playground until we finally complete this PR.

  1. yarn global add electron-forge-template-vite
  2. yarn create electron-app --template=vite

image

@erickzhao erickzhao changed the title feat(wip): support vite template feat(template): add vite template Nov 24, 2022
@erickzhao
Copy link
Member

I removed the wip from the template because the check prevents CI from running until you remove it!

We're thinking of removing it in the long term now that Draft PRs are a builtin GitHub feature.

@erickzhao
Copy link
Member

@caoxiemeihao can you rebase with the latest main? I'll give it another review tomorrow!

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Nov 25, 2022

@erickzhao Yep! Good night!

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Nov 25, 2022

#3071 (comment)
But then again, I think it's ultimately necessary to flat the configuration, which would not only make it more flexible, but also reduce the complexity of Vite integration. Otherwise we might need to write a lot of built-in plugins to fit vite serve and vite build.

@sipt
Copy link

sipt commented Dec 2, 2022

@caoxiemeihao
index.html not found.

  1. yarn global add electron-forge-template-vite
  2. yarn create electron-app template --template=vite
  3. cd template
  4. yarn make
  5. open out/template-darwin-x64/template.app
    image
    image

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Dec 2, 2022

@sipt I will try to fix it tomorrow.


@sipt Hey! 👋
You can try use latest version.

@sipt
Copy link

sipt commented Dec 8, 2022

How to use 'yarn make' without copy source code? @caoxiemeihao
image

@erickzhao erickzhao self-requested a review December 9, 2022 00:52
@caoxiemeihao caoxiemeihao force-pushed the main branch 4 times, most recently from 6f68476 to ed62ab2 Compare December 18, 2022 09:27
@warrenbuckley
Copy link

Hiya 👋
I am just following along here, what is the latest status of this PR?
Can I expect it to be merged into Electron Forge soon @erickzhao @caoxiemeihao ?

@erickzhao
Copy link
Member

@warrenbuckley we're hopefully looking to getting this PR merged soon. Currently going through the code review process.

@martinszeltins
Copy link

@warrenbuckley we're hopefully looking to getting this PR merged soon. Currently going through the code review process.

Awesome, can't wait for this! Also, there should be a TypeScript option.

caoxiemeihao and others added 6 commits January 29, 2023 17:44
Co-authored-by: Black-Hole <158blackhole@gmail.com>
Co-authored-by: Black-Hole <158blackhole@gmail.com>
Co-authored-by: Black-Hole <158blackhole@gmail.com>
Co-authored-by: Black-Hole <158blackhole@gmail.com>
Co-authored-by: Black-Hole <158blackhole@gmail.com>
@BlackHole1 BlackHole1 mentioned this pull request Feb 3, 2023
3 tasks
@caoxiemeihao caoxiemeihao requested review from erickzhao and BlackHole1 and removed request for BlackHole1 and erickzhao February 4, 2023 00:28
@mmm8955405

This comment was marked as off-topic.

@erickzhao
Copy link
Member

I think CI is failing we bumped the version up to 6.1.0, but it should be aligned with the rest of the monorepo package versions (6.0.5). We use syncpack to enforce that:

https://www.npmjs.com/package/syncpack

The version number will be automatically bumped by Lerna during the publishing process!

Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @caoxiemeihao!

@erickzhao erickzhao added this pull request to the merge queue Feb 25, 2023
Merged via the queue into electron:main with commit 393709d Feb 25, 2023
@tuul-wq tuul-wq mentioned this pull request Feb 25, 2023
3 tasks
@martpie
Copy link

martpie commented Mar 4, 2023

Any hope to see this officially released soon? Very eager to give it a try :]

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.