Skip to content

fix(build): watch mode - replace rollup.watch with chokidar#3145

Closed
stafyniaksacha wants to merge 7 commits intovitejs:mainfrom
stafyniaksacha:fix-build-watch
Closed

fix(build): watch mode - replace rollup.watch with chokidar#3145
stafyniaksacha wants to merge 7 commits intovitejs:mainfrom
stafyniaksacha:fix-build-watch

Conversation

@stafyniaksacha
Copy link
Contributor

@stafyniaksacha stafyniaksacha commented Apr 25, 2021

Description

Using rollup.watch comes with some leaks: we can not watch for configuration changes neither control what is built.
This is an alternative approach that replace the use of rollup.watch with a custom chokidar that trigger internal build (so we can not have different behaviour while using the watch option)

Caveat:
This comes with a breaking change in the watcher configuration from RollupWatcher (rollup) to WatchOptions (chokidar)

Fix #3108
Fix #3068

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.

@patak-cat
Copy link
Member

About the breaking change, could we still accept rollup watch options and issue a deprecation warning?

  watch?: WatcherOptions | WatchOptions | null // WatcherOptions is deprecated

I think we could release this as a minor if we do this without breaking user setups.

@patak-cat
Copy link
Member

For reference, the conflicts were caused by #3043, that already fixes #3068

@antfu antfu added enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority) labels Apr 27, 2021
@antfu antfu changed the title Fix build watch: replace rollup.watch with chokidar fix(build): watch mode - replace rollup.watch with chokidar Apr 27, 2021
@stafyniaksacha
Copy link
Contributor Author

Should be ok now

@underfin
Copy link
Member

underfin commented May 9, 2021

I don't think it is a better fix, the rollup watch build will cache module transform result and other perfmance optimized. This fix will lost the optimzied of rollup. I think we should found the reason with issue then fixed them.

@CHOYSEN
Copy link
Contributor

CHOYSEN commented May 20, 2021

I think additional implementation of watch mode may bring other issues like affect this.addwatchfile in the plugin hook.

@patak-cat
Copy link
Member

@stafyniaksacha we discussed this with Evan about this approach, and @underfin has a point here. We should try to use rollup watch mode as in your original PR, as there are performance benefits. A few bugs were solved already by #3512 and related PRs. The plugin caches were not reset when rebuilding.
I think we should close this PR for the moment. Let's keep improving the rollup-based build mode at least until we hit a roadblock.

@patak-cat patak-cat closed this Jun 6, 2021
@stafyniaksacha stafyniaksacha deleted the fix-build-watch branch June 6, 2021 23:53
@stafyniaksacha stafyniaksacha restored the fix-build-watch branch June 6, 2021 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Styles not being recompiled when using --watch mode vite build --watch should check emptyOutDir

6 participants