feat(build): watch vite.config.js in --watch mode

[aleclarson]

Description

With this PR, the Vite config is reloaded and the Rollup watcher is recreated whenever the Vite config is changed while the build.watch option is true.

The watcher returned by the original build call is able to .close() the current watcher at any time.

import * as vite from 'vite'

const watcher = await vite.build({ watch: true })

// Later on... after changes to the config file

watcher.close() // this still works

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[Shinigami92]

Code looks good to me, I think it's not really possible to write tests for it, is it?

[patak-dev]

Couldn't re-starting the config file watcher end up missing change events? If the user saves the config file several times in a row, it may miss the change while it is restarting the build, no?

I think that a more robust solution would be to leave doBuild untouched and create the chokidar file watcher only once, closing it once every rollup watcher is closed.
Looks like we are also returning the rollup watcher that will end up being outdated. Should we return the chokidar watcher if there is a config file? Since we are going to be re-starting it, we shouldn't leak the rollup watcher in this case.

I'm thinking of something like this outside of doBuild (or wrapping it). Warning: untested code.

if (config.configFile) {
  const configWatcher = chokidar.watch(config.configFile)

  let activeWatchers = []
  const onClose = closedWatcher => {
    activeWatchers = activeWatchers.filter( entry => entry.watcher !== closedWatcher )
    if( activeWatchers.length === 0 ) {
      configWatcher.close()
    }
  }
  const outdateWatcherEntry = entry => {
    entry.watcher?.close()
    entry.outdated = true
  })

  configWatcher.once('close', () => {
    activeWatchers.forEach(outdateWatcherEntry)
  })

  configWatcher.on('change', () => {
    const oldWatchers = [...activeWatchers]
    
    // Push the new watcher entry before the promise to avoid closing the 
    // configWatcher while waiting for reload to finish
    const entry = { watcher: null, outdated: false }
    activeWatchers.push(entry)

    oldWatchers.forEach(outdateWatcherEntry)
    
    // Reload the config and create another watcher
    const watcherPromise = doBuild(inlineConfig)
    
    watcherPromise.then( watcher => {
      entry.watcher = watcher
      watcher.once('close', () => onClose(watcher))
      if( entry.outdated ) {
        watcher.close()
      }
    })
    
    watcherPromise.catch((error) => {
      process.emit('uncaughtException', error)
    })
  })

  return configWatcher
}
else {
  return doBuild(inlineConfig)
}

[aleclarson]

Couldn't re-starting the config file watcher end up missing change events? If the user saves the config file several times in a row, it may miss the change while it is restarting the build, no?

I fixed this by comparing configFile mtime before/after it's been loaded. I did some manual testing, and it's catching changes between restarts now. 4d90f20

Looks like we are also returning the rollup watcher that will end up being outdated.

We could use a Proxy to redirect method calls to the latest Rollup watcher. I thought it was "good enough" to just override its close method instead. If you think otherwise, you could write a follow-up PR. 👍

[aleclarson]

Actually, I'll see about the proxying bit. It shouldn't be difficult.

edit: Well, maybe we should think about exposing our own ViteWatcher that only has a close method. Or is there a good use case for listening to Rollup watcher events?

[patak-dev]

Ya, I think it is better to avoid exposing the rollup watcher. Users may think they can subscribe to events and these will be lost once the config file changes.

[aleclarson]

We could forward Rollup events from the last watcher to the first watcher. :)

[aleclarson]

In that case, this PR is ready to merge. If anyone tries to use the Rollup watch events, they'll open an issue when it doesn't work.

[patak-dev]

For reference, is there a reason for the current design where you need to manually check the config file mtime? Wouldn't it be more robust to create the chokidar watcher only once and keep it up until the end?

[aleclarson]

They're equal in robustness. The "problem" with your approach (besides my opinion that it's more complex) is that the chokidar watcher is recreated on each doBuild call, so we would need another doBuildWatch function to ensure the chokidar watcher is created just once. Additionally, returning the Rollup watcher keeps the door open for the ability to observe Rollup events in the future (when developers need it enough to open an issue about it).

[patak-dev]

Yes, at least myself I don't see having that extra wrapper for doBuild as an issue. But ya, if this works then it's fine, I let others check it out. We'll need approval from Evan anyway (although maybe for this particular feature it may be merged with enough approvals as there aren't any new config)

[aleclarson]

Yeah, I'm about 99.99% certain Evan wouldn't object to this PR. :P

The edge case you pointed out is a little difficult to test manually. I added logs to ensure the configFileLastModified check was being used when I saved twice fast enough. I can guarantee it works, so if y'all would rather avoid the hassle, we can just merge it.

[yyx990803]

I think the watch logic is a bit convoluted because we are setting up the watchers inside a function, and the watcher needs to call this function on change. We are doing this because we want to reuse all the shared build options resolving between normal build and watch mode.

I think we need to refactor the code to split the build --watch logic into a separate function so that:

doBuild
  -> resolve build options
  -> if (not watch mode) -> do normal build
  -> else
    -> call initial `watchBuild`
    -> setup config watcher (once)
       -> on change: teardown previous Rollup watcher, call `watchBuild` again

[bluwy]

Looks like this had gone stale, and there's now a new PR that resolves this too: #11962, which handles config file deps and env files too. Closing in favour of that one.