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

Fix: Wait until the last file is done bundling before starting Electron #166

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

CommandMC
Copy link
Contributor

In a setup with multiple entry files (for example, a preload and a main), the plugin would start Electron once the first one is done bundling. This is a problem, since the application might then not start at all / execute outdated code
With this PR, it now waits for all files to be done before launching Electron

I've had this happen in a repo with a fairly large main entry, compared to a smaller preload. The preload would finish, the plugin would try to start Electron before main is done; because main.js then didn't exist at all, Electron would immediately exit with an error, which in turn also cancelled the main build. Only manually creating the main.js file (with an utility like touch) would actually make the build work, which is rather cumbersome

Please note that this is my first time working on Vite plugins, please let me know if I've made any obvious mistakes. Thanks!

@caoxiemeihao
Copy link
Member

👋 Thx for your PR!
Regardless, you can customize the startup timing via the onstart option.
A simple way to determine the length of an array is inaccurate, as users can define multiple entry files in one entry.

@CommandMC
Copy link
Contributor Author

CommandMC commented Sep 29, 2023

Regardless, you can customize the startup timing via the onstart option

Right, but since the plugin aims for simplicity (and this is arguably a design flaw), I think it should do the "correct" thing by default

users can define multiple entry files in one entry

I see

  • Is this a supported use case? Doesn't seem to be mentioned anywhere in the README
  • As far as I can see it, this results in just one closeBundle event being fired (while we also just wait for one), so everything works out

Since we're waiting for as many closeBundle events as there are ElectronOptions provided, and one option = one viteBuild call, I don't see how this would cause issues in the first place

@caoxiemeihao
Copy link
Member

caoxiemeihao commented Oct 1, 2023

There may not be any design flaws, I just want the plugin maintenance cost to be low enough.

import electron, { startup } from 'vite-plugin-electron'

let largeReady = false

export default {
  plugins: [
    electron([
      {
        entry: 'electron/main.ts',
        onstart() { // Automatic startup can be prevented.
          largeReady && startup()
        },
      },
      {
        entry: 'electron/preload.ts',
        onstart() { // Automatic startup can be prevented.
          largeReady && startup()
        },
      },
      {
        entry: {
          foo: 'foo.ts',
          large: 'large.ts', // A large file here
        },
        vite: {
          plugins: [
            {
              name: 'custom-start',
              closeBundle() {
                if (!largeReady) {
                  largeReady = true
                }
                startup() // Delayed start
              },
            },
          ],
        },
      },
    ]),
  ],
}

@CommandMC
Copy link
Contributor Author

Oh I'm perfectly aware how you'd solve this on the user side. I'd just think that a build tool reporting that the build is done when it is, in fact, not done is a flaw of that build tool and shouldn't be up to the user to fix

Not counting the comment/whitespace, this is a 4 line patch. How much added maintenance cost are you expecting here?

@caoxiemeihao
Copy link
Member

caoxiemeihao commented Oct 1, 2023

Although Electron will start early before a large file(Preload) is built, when the large file is built, Electron will be restarted by default and will still work normally.
If you prevent "restart" via the onstart option, this behavior itself is uncontrollable.

If you must think this is a design flaw, I think a rs command can be provided to help users restart Electron just like Forge.

@CommandMC
Copy link
Contributor Author

when the large file is built, Electron will be restarted by default and will still work normally

That's the original problem: It isn't. If your larger files is Electron's main entrypoint (which it is in 99% of cases), your preload (or any other file that you've specified) will finish building, the plugin will then try to start Electron, which will then fail (as noted in my comment). This will then cancel the whole build, meaning no main.js will ever be built. The only way to get a project like this to start would be to manually create the output file

Once you've built a project once, it's true that Electron will restart once the build is done. However, this still leads to unnecessary "work" (starting Electron, only to restart it a couple seconds later) and potential issues (you're executing outdated code for that brief amount of time)

@CommandMC CommandMC force-pushed the fix/wait-for-all-files branch from 59a3994 to ab990b9 Compare October 1, 2023 13:55
@CommandMC
Copy link
Contributor Author

If you prevent "restart" via the onstart option, this behavior itself is uncontrollable.

Hm, I think I see what you mean here: Checking for equality in the condition breaks hot restart. This wasn't intentional. Should be resolved now

src/index.ts Outdated
leading to an "Unable to find Electron app at ..." error
*/
closeBundleCount++
if (closeBundleCount < optionsArray.length)
Copy link
Member

@caoxiemeihao caoxiemeihao Oct 1, 2023

Choose a reason for hiding this comment

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

It seems that we need to judge based on the number of entry files, so the judgment conditions here are not accurate.
It may be possible to detect how many entry files there are in the configResolved hook, if that is possible I will consider merge this PR.

e.g.

import electron from 'vite-plugin-electron'
import simpleElectron from 'vite-plugin-electron/simple'

export default {
  plugins: [
    // Case 1
    electron([
      {
        entry: {
          foo: 'foo.ts',
          bar: 'bar.ts',
        },
      },
    ]),

    // Case 2
    electron({
      entry: {
        foo: 'foo.ts',
        bar: 'bar.ts',
      },
    }),

    // Case 3
    electron({
      entry: [
        'foo.ts',
        'bar.ts',
      ],
    }),

    // Case: use the simple API
    simpleElectron(/* Same as above */),
  ],
}

Copy link
Contributor Author

@CommandMC CommandMC Oct 1, 2023

Choose a reason for hiding this comment

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

Let's see here:

  • For each option object provided, we wait for one event & call Vite's build function once
  • Vite calls Rollup's bundle.write or bundle.generate functions (see source code for Vite)
  • According to Rollup's Plugin development docs, one call to write/generate will result in exactly one closeBundle event

TLDR: The current implementation works

@caoxiemeihao
Copy link
Member

@CommandMC Hey 👋
I was pushed a PR here 👉 CommandMC#1

@caoxiemeihao caoxiemeihao merged commit 9a910e5 into electron-vite:main Nov 6, 2023
@caoxiemeihao caoxiemeihao mentioned this pull request Nov 6, 2023
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