[Proof of Concept] Move from `chokidar` to `sane`. #41

wtgtybhertgeghgtwtg · 2016-12-18T08:02:09Z

Proposed to close #36.

wtgtybhertgeghgtwtg · 2016-12-18T09:51:32Z

Tests are failing on OS X. I'll see if I can get access to a Mac or something to test.

SpaceK33z · 2016-12-18T09:53:08Z

@wtgtybhertgeghgtwtg, the tests on master are also failing on OS X (don't know if it's the same problem). That said, it would be awesome if you can fix that.

I'll start testing this soon.

SpaceK33z · 2016-12-20T11:17:30Z

Okay a first round of real-world testing:

It works correctly on macOS (and it's very fast!).
On a Vagrant box with a NFS share with polling enabled; this works.

However I noticed one issue on both tests: if I import a module that does not exist, it throws an error and exits the process.

Error: watch /Users/kees/dev/node_modules/asdfasdf ENOENT
    at exports._errnoException (util.js:1026:11)
    at FSWatcher.start (fs.js:1429:19)
    at Object.fs.watch (fs.js:1456:11)
    at NodeWatcher.watchdir (/Users/kees/dev/watchpack/node_modules/sane/src/node_watcher.js:144:20)
    at new NodeWatcher (/Users/kees/dev/watchpack/node_modules/sane/src/node_watcher.js:45:8)
    at sane (/Users/kees/dev/watchpack/node_modules/sane/index.js:17:12)
    at new DirectoryWatcher (/Users/kees/dev/watchpack/lib/DirectoryWatcher.js:49:17)
    at WatcherManager.getDirectoryWatcher (/Users/kees/dev/watchpack/lib/watcherManager.js:16:33)
    at WatcherManager.watchFile (/Users/kees/dev/watchpack/lib/watcherManager.js:26:14)
    at EventEmitter.<anonymous> (/Users/kees/dev/watchpack/lib/watchpack.js:35:49)
    at Array.map (native)
    at EventEmitter.watch (/Users/kees/dev/watchpack/lib/watchpack.js:34:28)
    at NodeWatchFileSystem.watch (/Users/kees/dev/cy-dashboard/frontend/node_modules/webpack/lib/node/NodeWatchFileSystem.js:52:15)
    at Watching.watch (/Users/kees/dev/cy-dashboard/frontend/node_modules/webpack/lib/Compiler.js:102:47)
    at Watching._done (/Users/kees/dev/cy-dashboard/frontend/node_modules/webpack/lib/Compiler.js:98:8)
    at /Users/kees/dev/cy-dashboard/frontend/node_modules/webpack/lib/Compiler.js:76:18

In my entry file I did import 'asdfasdf'. Interestingly this does NOT occur when I do import './asdfasdf'.

Furthermore, the ignored option seems to not work anymore. In poll mode people often want to do e.g. ignored: /node_modules/ to minimize memory usage.

SpaceK33z · 2016-12-20T11:33:29Z

To make sane use watchman, don't you need to add watchman: true in the options? It looks like sane automatically detects if watchman is installed and if it isn't, it'll fallback to the "normal" watcher.

wtgtybhertgeghgtwtg · 2016-12-20T13:41:19Z

Furthermore, the ignored option seems to not work anymore. In poll mode people often want to do e.g. ignored: /node_modules/ to minimize memory usage.

That might be a problem. ignored can take anything that works with anymatch (globs, regexen, functions), but sane only handles globs, and only as whitelists. And while that use case could still work (ignored: 'node_modules{,/**}', internally negate it to '!node_modules{,/**}'), it would be breaking.
The good news is that there is an issue and a PR for that kind of functionality, and the ideas seemed to be approved. I'll see if sane will accept a PR so that this can be done.

wtgtybhertgeghgtwtg · 2016-12-20T13:50:30Z

To make sane use watchman, don't you need to add watchman: true in the options? It looks like sane automatically detects if watchman is installed and if it isn't, it'll fallback to the "normal" watcher.

That was causing tests to fail, at least on my machine. I can do something like what jest does, but I had figured that using watchman would go behind a flag and be outside of the scope of this PR.

SpaceK33z · 2016-12-20T14:04:58Z

Aha, I had hoped sane did that automatically. I would prefer to not have a flag and do the same as what Jest does. But I agree, we could do this after this PR.

wtgtybhertgeghgtwtg · 2016-12-20T19:11:14Z

sane PR: amasad/sane#77

wtgtybhertgeghgtwtg · 2016-12-25T18:41:08Z

ignored should be working now.

However I noticed one issue on both tests: if I import a module that does not exist, it throws an error and exits the process.

Is there a test for this? What is the expected behavior?

SpaceK33z · 2016-12-28T09:56:54Z

Is there a test for this? What is the expected behavior?

Apparently there isn't a test for this, maybe in the webpack/webpack repo there is but I doubt it.

The expected behavior is that when I import a non-existent module in my entry file (import 'asdfasdf';), the watch process keeps running and the console logs an error about the non-existent module. Before applying this PR that error looks like this:

ERROR in ./src/entry.js
Module not found: Error: Can't resolve 'asdfasdf' in '/home/foobar/src'
 @ ./src/entry.js 11:0-18
 @ multi bundle

Note that the watch process keeps running (which it should). After applying this PR, the process stops immediately:

ERROR in ./src/entry.js
Module not found: Error: Can't resolve 'asdfasdf' in '/home/foobar/src'
 @ ./src/entry.js 11:0-18
 @ multi bundle
webpack: bundle is now VALID.
/home/foobar/node_modules/watch/main.js:67
    if (err) throw err;
             ^

Error: ENOENT: no such file or directory, stat '/home/foobar/node_modules/asdfasdf'
    at Error (native)
error Command failed with exit code 1. (<== this is from npm)

The module watch seems to be a dependency of sane.

wtgtybhertgeghgtwtg · 2017-01-01T21:55:04Z

I'm sorry, could you provide a repo showing this error? I'm having trouble reproducing it.

wtgtybhertgeghgtwtg · 2017-01-10T13:17:59Z

watch seems to only be used in poll mode. Is this an issue in other modes, as well?

SpaceK33z · 2017-01-20T13:48:19Z

@wtgtybhertgeghgtwtg sorry for the delay. I have created an example repo for you with instructions how to reproduce it. It seems to be also happening in other modes.

wtgtybhertgeghgtwtg · 2017-01-20T19:59:55Z

Thank you. It looks like it's because sane doesn't wrap fs.watch in a try-catch block like chokidar does. So chokidar will check to see if an error is ENOENT or ENOTDIR and swallow the error if it is not.
I'll submit a PR to sane to see if I can fix this.

xdissent · 2017-01-20T23:54:28Z

@SpaceK33z what platform/node version are you running?

Possibly related, webpack seems to totally bug out and try to watch everything if you try to import a nonexistent module. Adding a console.log('WATCHING', directoryPath) in the DirectoryWatcher constructor reveals this insanity (get it?):

> watchpack-pr-41-test@1.0.0 start /Users/xdissent/src/watchpack-pr-41-test
> webpack-dev-server

Project is running at http://localhost:8081/
webpack output is served from /
WATCHING /Users/xdissent/src/watchpack-pr-41-test
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/ansi-regex
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/debug
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/events
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/inherits
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/json3/lib
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/ms
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/process
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/punycode
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/querystring-es3
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/querystringify
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/requires-port
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/sockjs-client/lib
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/sockjs-client/lib/event
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/sockjs-client/lib/transport/browser
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/sockjs-client/lib/transport
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/sockjs-client/lib/transport/lib
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/sockjs-client/lib/transport/receiver
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/sockjs-client/lib/transport/sender
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/sockjs-client/lib/utils
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/strip-ansi
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/url-parse
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/url
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/webpack-dev-server/client
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/webpack/buildin
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules/webpack/hot
WATCHING /Users
WATCHING /Users/xdissent
WATCHING /Users/xdissent/src
WATCHING /Users/xdissent/src/watchpack-pr-41-test/node_modules
WATCHING /
Hash: f8f5c40ea786fce42b6a
Version: webpack 2.2.0
Time: 412ms
    Asset    Size  Chunks             Chunk Names
bundle.js  246 kB       0  [emitted]  main

Strangely, for me on Mac and linux, it doesn't attempt to watch node_modules/asdfasdf at all...

romanzenka · 2017-01-27T22:05:42Z

So I gave this pull request a try. When I had polling enabled

watchOptions: {
      aggregateTimeout: 300,
      poll: 1000
    },

.. I experienced exact same behavior - 15% CPU gone.

After I removed this setting from the config file, the file change detection still worked and CPU went to 0%.

SpaceK33z · 2017-01-29T11:26:22Z

@romanzenka awesome, so do I understand it correctly that you previously had to use polling mode because otherwise file change detection wouldn't work, but after this PR you don't even need to use polling mode anymore? That would be a really great benefit :).

romanzenka · 2017-01-29T21:48:46Z

@SpaceK33z Yes, that's what I am saying. Let me do a bit more research though to make sure that the fix was not due to something else I changed instead of just the Chokidar -> Sane switch.

romanzenka · 2017-01-29T22:43:44Z

@SpaceK33z Confirmed. I removed the patched version of watchpack and Windows went back to ignoring saves to files in my IDE (IntelliJ). Interestingly enough, if I do 'git revert', that change gets noticed properly.

alexander-doroshko · 2017-01-30T07:47:39Z

Windows went back to ignoring saves to files in my IDE (IntelliJ)

This commit fixed the problem of not detecting IntelliJ's 'safe write'. The fix is available since watchpack 1.1.0. So Webpack 2.0 (that depends on watchpack 1.2.0) + webpack-dev-server do not have any problems with IntelliJ IDEA. As I understand, watching package doesn't matter, it works well both with chokidar and sane.

geowarin · 2017-01-30T10:36:23Z

Just a testimony, I have been digging into the watchpack source code and really think it needs a revamp.
I'm not really sure why there is a tree-like structure of watchers.
The FS_ACCURACY thing is also really strange to me.
Its adjustment could at least be moved to the initial scan for it to be accurate early in the watch process.

I also had the strangest problem running webpack in watch mode and trying running it in the JVM: segfaults when using chokidar and no problem running sane.

So I'm endorsing this PR 100%.
As far as I'm concerned, I chose to rewrite my own watcher using webpack's context option as the watch directory. The code is much simpler and it seems to cover 99% of use cases.

I also noticed concurrency problems when running tests with high speed rewrites of watched files.
My watcher would pick up the changes but webpack would not always trigger a recompilation.
I'm not sure if it's a known problem, I could work on a PR reproducing the issue if it isn't.

wtgtybhertgeghgtwtg · 2017-01-30T19:49:34Z

I agree, but that's out of the scope of this PR. I was planning on proposing a rewrite (and a node>=4 refactor) after this PR.

codecov · 2017-02-12T12:34:35Z

Codecov Report

Merging #41 into master will decrease coverage by -3.35%.
The diff coverage is 62.96%.

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
- Coverage   94.98%   91.64%   -3.35%     
==========================================
  Files           3        3              
  Lines         339      323      -16     
  Branches       85       72      -13     
==========================================
- Hits          322      296      -26     
- Misses         17       27      +10

Impacted Files	Coverage Δ
lib/DirectoryWatcher.js	`89.37% <62.96%> (-5.25%)`	❌

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1309419...ef4a342. Read the comment docs.

amasad · 2017-02-13T07:13:04Z

lib/DirectoryWatcher.js

-		atomic: false,
-		alwaysStat: true,
-		ignorePermissionErrors: true,
+	this.watcher = sane(directoryPath, {


According to this repro repo it seems like this can be called with non-directory or a non-existent directory. Sane will have to throw an error here because it's an obvious user error to call sane on a non-directory. See my comments:

Do not throw on missing directory/file. amasad/sane#87

Swallow ENOENT and ENOTDIR errors for fs.watch. amasad/sane#88

Looking at the stack from the repro repo I think you should filter the files before mapping them to the watcherManager in watchpack.js which seems like it will solve the problem for you.

You're completely right and I'm kinda embarrassed that I didn't figure that out earlier.
I wrote a cheap one-line filter for this and it worked. I'll see if I can get a more efficient one hammered out by tomorrow.

wtgtybhertgeghgtwtg · 2017-02-16T04:13:48Z

This latest commit brings good news and bad news.
The good news is that https://github.com/SpaceK33z/watchpack-pr-41-test now works.
The bad news is that the solution exacerbates a race condition in watchpack, which is why Travis is failing.
I'll try and remove the race condition with a queue system or something. the tests depend on nonexistent files being watched.

xdissent · 2017-02-16T05:49:17Z

Friendly reminder about wtgtybhertgeghgtwtg/watchpack#1 I think you've still got some hidden issues in this PR, which may or may not be exposed by the tests. For ex, these setFileTime() calls should be setDirectory() for directories

Plus it has watchman =)

Fix sane

jsf-clabot · 2017-02-16T05:53:54Z

All committers have signed the CLA.

wtgtybhertgeghgtwtg · 2017-02-18T08:54:01Z

The problem is that the failing tests rely on watching a file that doesn't exist yet. @SpaceK33z, is watchpack supposed to be able to do that?

SpaceK33z · 2017-02-18T10:48:27Z

@wtgtybhertgeghgtwtg I don't know, I'm only managing the issues / PR's for this repo a bit. However, if that test succeeded before, it should keep working 😄.

wtgtybhertgeghgtwtg · 2017-02-20T08:38:32Z

The tests that are failing are Watchpack should watch a single file and Watchpack should watch multiple files. There's no indication that it's supposed to be watching a file that does not yet exist. It's not in the name of the tests, it's not documented, and I don't see where webpack would use that functionality. I might be able to patch that in, but it would be very brittle and might break other things; I don't want to do that unless it's absolutely necessary.

romanzenka · 2017-02-20T15:15:18Z

Well let's think about it - under which circumstance would you want Watchpack to trigger rebuild on a file that was freshly created? When I am splitting a file into two, I can easily delete one half of file A, save, create file B, save. If Watchpack does not notice that file B appeared, it will be stuck on the failed compile with one half of A. So I would argue that being able to notice file being created is important.

wtgtybhertgeghgtwtg · 2017-02-20T15:25:04Z

Why would it need to watch file B, specifically, before it existed? What if I had named it file C?

SpaceK33z · 2017-02-20T15:28:31Z

@wtgtybhertgeghgtwtg, perhaps @TheLarkInn or @sokra can you give more info about this.

romanzenka · 2017-02-20T15:31:17Z

Well just think what we are trying to accomplish here. "If source code changes, recompile". Source code can change by people modifying files, creating files or deleting files. The mental model is "If I change it, recompile will trigger". If you do not notice files being created, as a programmer I will have to think "After I add this file I must remember to modify some other file that already existed for no reason, otherwise recompile will not trigger".

sokra · 2017-02-20T15:34:25Z

Consider this scenario:

You add a require("B") to file A.
The build fails because file B is missing.
This is a watch build so A and B is watched
You add file B.
Watch detects that you added file B.
Recompile and everything is fine now.

If you can't watch non-existing files it would not detect adding of file B.

wtgtybhertgeghgtwtg · 2017-02-20T20:10:14Z

How about creating a new major version of watchpack with a slightly different API. Name, instead of

watch(files, dirs, startTime)

it'd be

watch(files, dirs, missing, startTime)

webpack already knows what files are missing, it just concats them to files. Otherwise, watchpack has to existsSync every filename to determine if it exists or not.

jonashartwig · 2017-04-04T07:59:22Z

Hi everyone. I see a good discussion going on here. However it seems to have died February 23rd. So what should we do now?

Move from chokidar to sane.

70cc2fe

wtgtybhertgeghgtwtg mentioned this pull request Dec 18, 2016

Consider moving from chokidar to sane. #36

Closed

Remove literal.

c33125f

Possibly improve lost coverage, trigger Travis.

7b426dc

Remove dot, merge if statements.

6bb36d2

wtgtybhertgeghgtwtg mentioned this pull request Dec 24, 2016

Add ignored option. amasad/sane#79

Merged

wtgtybhertgeghgtwtg added 2 commits December 25, 2016 10:40

Fix ignored.

e603cbd

Rebase.

32976c9

wtgtybhertgeghgtwtg added 2 commits January 1, 2017 13:09

Rebase.

28ba268

Fix conflicts.

380cac1

wtgtybhertgeghgtwtg mentioned this pull request Jan 20, 2017

Do not throw on missing directory/file. amasad/sane#87

Closed

bebraw mentioned this pull request Jan 26, 2017

Webpack-dev-server on Windows consumes ~15% of CPU just watching for file changes webpack/webpack#4114

Closed

amasad reviewed Feb 13, 2017

View reviewed changes

wtgtybhertgeghgtwtg added 2 commits February 15, 2017 19:16

Merge remote-tracking branch 'upstream/master'

23099fe

Check if file exists before watching.

437e303

Fix sane

fb28b17

Merge pull request #1 from xdissent/sane-pr

ef4a342

Fix sane

selfup mentioned this pull request Feb 23, 2017

Webpack dev server takes up a lot of memory (751.9MB) for GitLab CE webpack/webpack#4363

Closed

4 tasks

d4rkr00t mentioned this pull request Oct 18, 2017

Bail out from watching earlier using watchOptions.ignored #58

Closed

wtgtybhertgeghgtwtg closed this Jan 27, 2019

[Proof of Concept] Move from chokidar to sane. #41

[Proof of Concept] Move from chokidar to sane. #41

Conversation

wtgtybhertgeghgtwtg commented Dec 18, 2016

wtgtybhertgeghgtwtg commented Dec 18, 2016

SpaceK33z commented Dec 18, 2016 • edited Loading

SpaceK33z commented Dec 20, 2016 • edited Loading

SpaceK33z commented Dec 20, 2016

wtgtybhertgeghgtwtg commented Dec 20, 2016

wtgtybhertgeghgtwtg commented Dec 20, 2016 • edited Loading

SpaceK33z commented Dec 20, 2016

wtgtybhertgeghgtwtg commented Dec 20, 2016 • edited Loading

wtgtybhertgeghgtwtg commented Dec 25, 2016

SpaceK33z commented Dec 28, 2016 • edited Loading

wtgtybhertgeghgtwtg commented Jan 1, 2017

wtgtybhertgeghgtwtg commented Jan 10, 2017

SpaceK33z commented Jan 20, 2017

wtgtybhertgeghgtwtg commented Jan 20, 2017

xdissent commented Jan 20, 2017

romanzenka commented Jan 27, 2017 • edited Loading

SpaceK33z commented Jan 29, 2017

romanzenka commented Jan 29, 2017

romanzenka commented Jan 29, 2017

alexander-doroshko commented Jan 30, 2017

geowarin commented Jan 30, 2017

wtgtybhertgeghgtwtg commented Jan 30, 2017

codecov bot commented Feb 12, 2017 • edited Loading

Codecov Report

amasad Feb 13, 2017

Choose a reason for hiding this comment

wtgtybhertgeghgtwtg Feb 16, 2017

Choose a reason for hiding this comment

wtgtybhertgeghgtwtg commented Feb 16, 2017 • edited Loading

xdissent commented Feb 16, 2017

jsf-clabot commented Feb 16, 2017 • edited Loading

wtgtybhertgeghgtwtg commented Feb 18, 2017

SpaceK33z commented Feb 18, 2017

wtgtybhertgeghgtwtg commented Feb 20, 2017

romanzenka commented Feb 20, 2017

wtgtybhertgeghgtwtg commented Feb 20, 2017

SpaceK33z commented Feb 20, 2017

romanzenka commented Feb 20, 2017

sokra commented Feb 20, 2017 • edited Loading

wtgtybhertgeghgtwtg commented Feb 20, 2017

jonashartwig commented Apr 4, 2017

[Proof of Concept] Move from `chokidar` to `sane`. #41

[Proof of Concept] Move from `chokidar` to `sane`. #41

SpaceK33z commented Dec 18, 2016 •

edited

Loading

SpaceK33z commented Dec 20, 2016 •

edited

Loading

wtgtybhertgeghgtwtg commented Dec 20, 2016 •

edited

Loading

wtgtybhertgeghgtwtg commented Dec 20, 2016 •

edited

Loading

SpaceK33z commented Dec 28, 2016 •

edited

Loading

romanzenka commented Jan 27, 2017 •

edited

Loading

codecov bot commented Feb 12, 2017 •

edited

Loading

wtgtybhertgeghgtwtg commented Feb 16, 2017 •

edited

Loading

jsf-clabot commented Feb 16, 2017 •

edited

Loading

sokra commented Feb 20, 2017 •

edited

Loading