do not register a watcher on basedir

[xianwill]

On OS X, watchify is getting stuck in constant recompile when running from command watchify . -o myfile.js -d -v. I threw logging in and the basedir is the trigger. Seems like chokidar is raising the change event on the basedir -- as a result of the outfile being written or maybe a timestamp change on the folder? Oddly, have not seen the same behavior on Windows.

Either way, checking the file to see if it is the basedir before registering a watcher seems to resolve the issue (line 96).

[xianwill]

FYI - looks like this fixes #190 which came in about the same time.

[ahdinosaur]

looks good to me as i can confirm it fixes #190. :+1:

@wilcosprey you might want to reduce your commit to only a single line addition on line 96 instead of also including 17 whitespace changes (including whitespace at the same level of the surrounding code is an existing style choice, not an accident). :tea: (EDIT: such as https://github.com/holodex/watchify/commit/1e26dc06b38601b95d88efca3b5e1e66446d25bc).

[zertosh]

@wilcosprey If you specify the entry file, instead of using . (e.g. watchify index.js -o myfile.js -dv), does the bug still happen?

[xianwill]

@zertosh Nope. The bug only occurred when using "."

[zertosh]

@wilcosprey I thought so. I'm crazy swamped right now so I can't dig deep into this until this weekend. I'm concerned about the value of b._mdeps.top.basedir. That basedir can be something else that is not process.cwd(). Is there a reason why specifying the entry file is not preferred to simply .?

[xianwill]

@zertosh No problem at all. I am actually specifying the entry file in my own usage now so this isn't affecting me. I only submitted the PR because (a) I'm loving me some browserify and watchify and thought it would be fun to contribute, and (b) there are a lot of docs, tutorials and seed templates out there w/ the . example and I'd hate to see the constant rebuild discourage adoption.

As far as the value of the basedirnot being cwd, I don't see that as a problem as long as its always the basedir of the entry file. I believe the bug occurs when the outfile is written to a subdir of the basedir when the basedir is in the watch list. The outfile being removed and re-added to the folder seems to cause a change event. Your link does give me some food for thought though. If I can get some time today I'll test a few other scenarios that occur to me.

[xianwill]

@zertosh I'm going to close this PR. I've thought about this more and I think the examples out there that are using watchify . -o js/bundle.js are just bad examples. You just shouldn't stick your bundle in a subdirectory of a directory you are watching. It doesn't make sense. Either watch the entry file instead of the parent directory OR put your bundle outside of the watched hierarchy (e.g. dist sibling of src). If the user wants to watch cwd, watchify should absolutely watch that folder for changes, otherwise, new files added to that folder would not trigger a rebuild.

[ahdinosaur]

I'm going to close this PR. I've thought about this more and I think the examples out there that are using watchify . -o js/bundle.js are just bad examples. You just shouldn't stick your bundle in a subdirectory of a directory you are watching. It doesn't make sense. Either watch the entry file instead of the parent directory OR put your bundle outside of the watched hierarchy (e.g. dist sibling of src). If the user wants to watch cwd, watchify should absolutely watch that folder for changes, otherwise, new files added to that folder would not trigger a rebuild.

in my opinion, this is still a bug. in the context of node, . is referring to the module in the current directory, not the directory itself. my understanding is that watchify watches the files that are part of the require tree of the module, not the directories themselves, as for example i don't want to rebuild my bundle.js just because i add a new image to my project assets directory, which is the same for not wanting to rebuild my bundle.js just because the bundle.js changed in my project assets directory.

at the very least, if we don't consider this a bug, this is unexpected and undocumented behavior.

I'm crazy swamped right now so I can't dig deep into this until this weekend.

no rush @zertosh, thanks for your help! the hotfix in this PR works well enough for me, thanks to @wilcosprey.

[xianwill]

@ahdinosaur You make some good points. It might still be a bug. It would need to be solved in a different way though because my previous PR would break a scenario where you wanted watchify to pick up new js files that you put in your watch directory.

I have another idea I'll try out real quick.

[xianwill]

@zertosh and @ahdinosaur - I think I may have found a happy middle path. I just moved the one-liner guard so that the basedir is watched but does not invalidate the build. That way, chokidar watches the folder and new files that are added do get picked up and have their own watchers registered, but a changed property on the directory itself does not actually trigger a rebundle.

Its still using the _mdeps basedir, but from the code I've seen so far (though my time was short today - I'll try to look deeper tomorrow) this seems like it should always be the right dir.

[zertosh]

@wilcosprey I think this issue might get solved by a fix to another problem. Can you give https://github.com/substack/watchify/pull/199 a quick try?

[xianwill]

@zertosh Sorry for the delay. Long day at work.

Just tested that branch. It does eliminate the infinite rebuild problem, but it looks like it only watches the main module and its dependencies. Maybe thats right -- just wasnt what I was expecting. So if I have this directory structure:

mydir -> a.js -> b.js

and package.json specifies a.js as the main module, changes in b.js are not noticed unless b is required by a. Is that the right behavior? I guess my initial assumption was that changes in any js file under the watched directory (required by main or not) would be watched.

[mattdesl]

and package.json specifies a.js as the main module, changes in b.js are not noticed unless b is required by a. Is that the right behavior?

This is the correct behaviour. b.js is not associated with your bundle so it isn't watched until a.js requires it.

[xianwill]

works for me then. looks like #199 resolves this issue.

[xianwill]

closing this PR since #199 resolves the issue.

[zertosh]

@wilcosprey Glad it works. Unfortunately https://github.com/substack/watchify/pull/199 feels like a hack. We're trying to figure out how to standardize some of the internals in browserify so this doesn't happen again in https://github.com/substack/node-browserify/issues/1203. So, I'm not sure if this particular fix will see the light of day, but hopefully something will soon.

[jeffbski]

+1 I just got hit by this too. I was using '.' for my watchify npm run script just like I use '.' for my browserify npm run script. Since the package.json contains the name of the main, we should be able to use '.'

Otherwise have to duplicate this information around which is not good.