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(plugin-vite): hot-restart 🔥 #3203

Closed
wants to merge 4 commits into from

Conversation

caoxiemeihao
Copy link
Member

@caoxiemeihao caoxiemeihao commented Apr 1, 2023

  • 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:

TODO:

  • write related test


2023-08-09

  1. Hot restart Main process, this means restart the entire Electron App. ✅
  2. Hot reload Preload scripts, refresh Renderer process can be reload the corresponding Preload script, it's will become better DX. ✅
  3. HMR Renderer process, power by Vite. ✅

Custom restart or reload by forge.config.js

// forge.config.js

module.exports = {
  plugins: [
    {
      name: '@electron-forge/plugin-vite',
      config: {
        build: [
          {
            entry: 'src/main.js',
            config: 'vite.main.config.mjs',
            
            // Disable auto restart
            // restart: false,

            // Custome hot restart
            restart(args) {
              // Start Electron App after do something...
              args.restart();
            },
          },
          {
            entry: 'src/preload.js',
            config: 'vite.preload.config.mjs',

            // Hot reload
            restart(args) {
              // When a Preload script is rebuilt,
              // users can refresh the Renderer process by `ViteDevServer`,
              // instead of restart the entire Electron App.
              args.renderer.find(({ config }) => config.name === 'main_window').reload();
            },
          },
        ],
        renderer: [
          {
            name: 'main_window',
            config: 'vite.renderer.config.mjs',
          },
        ],
      },
    },
  ],
};

@caoxiemeihao caoxiemeihao requested a review from a team as a code owner April 1, 2023 10:25
@caoxiemeihao caoxiemeihao force-pushed the feat/hot-restart branch 2 times, most recently from e6f7c9a to e243a4b Compare April 5, 2023 00:01
@grsky360
Copy link

grsky360 commented Apr 5, 2023

I tried this PR yesterday, and it looks great.
Just a small question, is there any way to not redirect my focus window to the restarted app?
I'm using WebStorm, everytime I changed something without over, the app restarted... And I don't want to close auto save function...

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Apr 5, 2023

At present, hot-restart just a simple-helper for dev.
Any time, if you want full custom the hot-restart logic you just emit rs command in the main process at the right time :)

process.emit('rs') // restart

TODO:

  • enhance hot-start

@GitMurf
Copy link

GitMurf commented Apr 24, 2023

I am very bad at understanding where PRs stand ;) is this finished and will be in the next release?

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Apr 25, 2023

This feature changes the API design of forge.config.js and it should be seriously considered.
In Vite, it might be better to use a peparate plugin, for now you can try to use the existing plugins.

@GitMurf 👉 Here

// vite.main.config.mjs    - For Electron Main
// vite.preload.config.mjs - For Preload Scripts

import { defineConfig } from 'vite';
import { restart } from 'electron-forge-plugin-vite/plugin';

// https://vitejs.dev/config
export default defineConfig({
  plugins: [restart()],
});

@BlackHole1
Copy link
Member

is this finished and will be in the next release?

@GitMurf We cannot guarantee that. Currently, it appears that @caoxiemeihao still has some features that have not been implemented, so we need to wait until they are completed before conducting a code review.

@caoxiemeihao caoxiemeihao changed the title feat(plugin-vite): hot-restart 🔥 wip(plugin-vite): hot-restart 🔥 Apr 26, 2023
@caoxiemeihao caoxiemeihao changed the title wip(plugin-vite): hot-restart 🔥 feat(wip(plugin-vite)): hot-restart 🔥 Apr 26, 2023
@caoxiemeihao
Copy link
Member Author

@erickzhao erickzhao marked this pull request as draft April 26, 2023 17:56
@ferminc
Copy link

ferminc commented Aug 12, 2023

any update on this?

@caoxiemeihao
Copy link
Member Author

any update on this?

This PR will reopen when #3178 is merged :)

@caoxiemeihao caoxiemeihao changed the title feat(wip(plugin-vite)): hot-restart 🔥 feat(plugin-vite): hot-restart 🔥 Aug 19, 2023
@caoxiemeihao caoxiemeihao requested a review from a team August 19, 2023 14:51
@caoxiemeihao caoxiemeihao marked this pull request as ready for review August 21, 2023 03:48
});

await viteDevServer.listen();
viteDevServer.printUrls();
// viteDevServer.printUrls();
Copy link
Member

Choose a reason for hiding this comment

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

question: is this commented out intentionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! When I run test on my local machine, some verbose log print, so commented it.

Copy link
Member

Choose a reason for hiding this comment

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

Could we remove this before we ship it to users then?

Copy link
Member Author

@caoxiemeihao caoxiemeihao Sep 8, 2023

Choose a reason for hiding this comment

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

The logs removed here are only for the CI test environment and will not affect the production environment.
Do we need to keep this log in the test environment?

Copy link
Member

@BlackHole1 BlackHole1 Oct 13, 2023

Choose a reason for hiding this comment

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

Do we need to keep this log in the test environment?

No. If others need it, they can add it themselves.

packages/plugin/vite/src/VitePlugin.ts Show resolved Hide resolved
packages/plugin/vite/src/VitePlugin.ts Outdated Show resolved Hide resolved
packages/plugin/vite/src/util/plugins.ts Show resolved Hide resolved
* renderer.find(({ config }) => config.name === 'main_window').reload();
* ```
*/
renderer: {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
renderer: {
renderers: {

Should this be renderers since it's a list of all renderer processes?

Copy link
Member Author

@caoxiemeihao caoxiemeihao Sep 8, 2023

Choose a reason for hiding this comment

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

How about this API design? 🤔
It looks little simper.

export interface VitePluginBuildConfig {
  restart?: (rs: (options?: { reload: 'main_window' }) => void) => void;
}

Usage

// Restart App
restart(rs) {
   rs();
};

// Reload Renderer process
restart(rs) {
   rs({ reload: 'main_window' });
};

Copy link
Member

Choose a reason for hiding this comment

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

I think @erickzhao means that because this is an array, it would be better to use plural for words.

) {
const restart = () => {
// https://github.com/electron/forge/blob/v6.1.1/packages/api/core/src/api/start.ts#L204-L211
process.stdin.emit('data', 'rs');
Copy link
Member

Choose a reason for hiding this comment

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

@MarshallOfSound pointed out to me that using stdin here is a bit of a hack here and there are edge cases where process.stdin could be null.

Ideally, we should instead expose additional start logic from the @electron-forge/core API to expose this functionality so that plugins can hook into it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay! This does seem a little dangerous. 🚨

});

await viteDevServer.listen();
viteDevServer.printUrls();
// viteDevServer.printUrls();
Copy link
Member

Choose a reason for hiding this comment

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

Could we remove this before we ship it to users then?

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Feb 9, 2024

Implemented in #3468

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.

6 participants