Skip to content

Rebuild project on package update (#2956) - #3936

Closed
maciej-ka wants to merge 1 commit into
react:nextfrom
maciej-ka:rebuild-on-package-update
Closed

maciej-ka wants to merge 1 commit into
react:nextfrom
maciej-ka:rebuild-on-package-update

Conversation

@maciej-ka

@maciej-ka maciej-ka commented Jan 29, 2018 •

Copy link
Copy Markdown
Contributor

Posting this to get early feedback.

My concerns are:

  • fs.watch has few problems
  • current implementation triggers duplicate rebuild after yarn add of a missing module

This is my first dive into webpack related source.
❤️ for any pieces of advice.

{ persistent: false, recursive: true },
() => {
clearTimeout(timeout);
timeout = setTimeout(() => compiler.run(() => {}), 1000);

@maciej-ka maciej-ka Jan 29, 2018 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In this approach, the fs.watch will be triggered, most likely, a lot of times on yarn add or yarn upgrade. Timeout here is to group these events and avoid rebuilding after a change of every file.

}

apply(compiler) {
fs.watch(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not use https://github.com/webpack/watchpack? also i think watching the whole files in node_modules would be very expensive, can we just watch all the package.jsons inside it? and ignore some folders that would be changed on runtime like .cache

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi, great to see you!
Webpack watch is current direction and changing a RegExp used to ignore node_modules.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, great to see you too!
Ah, I see, that would be better! would the ignored setting accept a function? I imagine it would be easier to read than a regex with lots of condition. Reading the docs it only accept regex or anymatch, but in some place like loaders' test it accept a function like (filePath) => filePath.endsWith('.js')

@maciej-ka

Copy link
Copy Markdown
Contributor Author

I'm experimenting with changing this watch ignore definition into

  return new RegExp(
    `^(?!${escape(
      path.normalize(appSrc + '/').replace(/[\\]+/g, '/')
    )})(?!.*package\.json$).+/node_modules/`

Which, I checked in node REPL, should ignore all paths in node_modules except package.json files. Sadly, it doesn't seem to change anything in current next branch or in a custom plugin.

Still inspecting. (I started a friendship with Code Studio Node debugger to solve this puzzle)

@gaearon

gaearon commented Feb 2, 2018

Copy link
Copy Markdown
Contributor

fs.watch is definitely bad, so we shouldn't use it.

I think it's a bit weird that we have this problem in the first place. I distinctly remember webpack being able to pick up changes to the installed packages. What happened?

@maciej-ka

Copy link
Copy Markdown
Contributor Author

We'll certainly use evil.fs.watch but just hidden in dependencies: webpack -> watchpack -> chokidar which is a wrapper for fs.watch.

This happened. CRA has a webpack setup to ignore watching of node_modules. It was added to solve 293.

@maciej-ka

maciej-ka commented Feb 9, 2018 •

Copy link
Copy Markdown
Contributor Author

Seems that webpack doesn't observe package.json files, unless they are imported.
So the plan to use webpack watcher to observe only package.json (by tweaking ignore regexp) is probably not valid.

In scenario:

  1. create-react-app /tmp/demo1
  2. cd demo1
  3. yarn add semver-console@0
  4. add import 'semver-console' into App.js
  5. turn on webpack watcher on node_modules by commenting out these lines in tmp/demo1/node_modules/react-scripts/config/webpackDevServer.config.js
  6. yarn start

While server is running, any change in node_modules/semver-console/index.js will trigger reload. But change in package.json will not be noticed by watcher.

Confirmed this with debugger: At this point webpack has a list of files to watch in files variable which includes .../semver-console/index.js but not a package.json file.

The observed files are calculated by visiting modules tree and collecting a list of files that are really used.

@maciej-ka

Copy link
Copy Markdown
Contributor Author

What's next ...

a) There is a PR to bring back watching node_modules on configurations where it would not cause a slowdown.
b) It would be possible to write a webpack plugin which uses chokidar to observe just package.json files.

@petetnt

petetnt commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

Hey @maciej-ka!

#3982 was created from some off-the-band discussions about the issues the ignore node_modules approach introduces and how to solve them.

For example CRA now supports monorepos so we should be able to handle multitudes of node_modules. fs.watch itself isn't bad on platforms that have high-performance file watchers (ie. not OSX) and chokidar provides escape hatches on situations where polling should be enabled (virtual drives, filewatching not working for some other reason etc.) which in return kill performance on platforms where it should just play nicely.

Would love if you reviewed #3982 and told me what you think about it ^^

@maciej-ka

Copy link
Copy Markdown
Contributor Author

3982 is so cool, because it would rebuild in both after adding and upgrading dependency. If I understand this: chokidar states that on both OSX and *nginx there will be no high CPU, however for the *nginx scenario it's advised to watch with a moderation.

Webpack plugin to watch package.json using chokidar could be a fallback. I'll update this PR.

@Timer Timer closed this Sep 26, 2018
@Timer Timer reopened this Sep 26, 2018
@gaearon gaearon closed this Nov 1, 2018
@lock lock Bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants