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(build): watch mode - replace rollup.watch with chokidar #3145

Closed
wants to merge 7 commits into from

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.

packages/vite/src/node/cli.ts Outdated Show resolved Hide resolved
packages/vite/src/node/build.ts Show resolved Hide resolved
@patak-dev
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-dev
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

packages/vite/src/node/build.ts Outdated Show resolved Hide resolved
@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-dev
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-dev patak-dev 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