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

Watching symlinks [feature] #31

Closed
vojtajina opened this issue Dec 12, 2012 · 33 comments
Closed

Watching symlinks [feature] #31

vojtajina opened this issue Dec 12, 2012 · 33 comments

Comments

@vojtajina
Copy link
Contributor

Chokidar might watch symlinks, something like http://stackoverflow.com/questions/9364514/how-to-watch-symlinked-files-in-node-js-using-watchfile

The ideal would be probably watching both the symlink and the real file (and update real file watcher, if the symlink changes).

I guess it might be as opt-in under a config option...

@paulmillr
Copy link
Owner

any particular reason to make it opt-in and not default behavior?

@vojtajina
Copy link
Contributor Author

@paulmillr you are right, it should be default behavior

@matthewwithanm
Copy link

I'm currently playing with Unitech/PM2 (which uses chokidar to trigger restarts on file changes). My use case is a symlink-based deployment strategy—where a symlink points to the current release directory. Since neither the symlinked directory nor any files in it ever change (only the symlink), and chokidar is following symlinks, pm2 will never be notified of a change.

I found #56 and it seems like the more recent thinking is that the current behavior should be retained (no breaking changes). Given that, is the way forward to add a followSymlinks option that defaults to true? (My thinking is that pm2 would then need to expose access to that option.) If so, I can try to put together a PR. Or if there's a better way, I can try to put together a PR to do that (:

Thanks!

cc @Unitech

@es128
Copy link
Contributor

es128 commented Oct 30, 2014

@matthewwithanm Yes, improved symlink functionality is desired before bumping to 1.0.0. PR welcome.

Adding a followSymLinks option is a good start, but I suspect more than that will be needed to do this well. Haven't taken time yet to fully think it through.

@scamden
Copy link
Contributor

scamden commented Nov 17, 2014

@es128 +1 on this issue, would be amazing to be able to use symlinks with watchify

@es128
Copy link
Contributor

es128 commented Dec 1, 2014

Apparently this was not the case when this issue was written, but as of now (testing with node 0.10.33), fs.watch and fs.watchFile transparently follow symlinks, and chokidar emits events as I'd expect, including when the symlink is updated.

What's left is to attain parity for fsevents watching mode.

@matthewwithanm
Copy link

Personally, I'd like to be able to watch the symlinks themselves; i.e. not transparently follow symlinks.

@es128
Copy link
Contributor

es128 commented Dec 2, 2014

In the use case you described above, if the transparent following is made to work correctly, you'd end up getting unlink events for everything under the previously linked path, and add events for the new ones. So it would resolve the situation you described about not being notified of changes at all.

The current behavior in fsevents mode is more like watching the link itself, so there'd be a single change event. So the first priority is achieving consistency between modes.

Then, I'd like to be able to offer an option turn off the transparent following and emit on the symlinks themselves. But since the fs-based modes seem to have baked-in following it may be challenging to find a good way to do that.

es128 added a commit that referenced this issue Dec 3, 2014
es128 added a commit that referenced this issue Dec 3, 2014
es128 added a commit that referenced this issue Dec 4, 2014
@es128
Copy link
Contributor

es128 commented Dec 4, 2014

But since the fs-based modes seem to have baked-in following it may be challenging to find a good way to do that.

Turns out it wasn't that difficult after all: 284148b

@matthewwithanm
Copy link

👍 Cool!

es128 added a commit that referenced this issue Dec 4, 2014
es128 added a commit that referenced this issue Dec 4, 2014
@es128
Copy link
Contributor

es128 commented Dec 4, 2014

Symlink support is looking pretty good in the master branch. Closing the issue.

The symlink-specific tests fail on Windows, but I suspect nobody is going to care.

@es128 es128 closed this as completed Dec 4, 2014
@scamden
Copy link
Contributor

scamden commented Dec 4, 2014

dude you are awesome. thanks for your fast fixes!

@es128
Copy link
Contributor

es128 commented Dec 4, 2014

@scamden It would be awesome if you could give the master branch code a try under watchify and confirm whether it behaves the way you'd like.

@scamden
Copy link
Contributor

scamden commented Dec 4, 2014

so like fork watchify and set the chokidar package json entry to hit your github repo? (sorry a bit of a noob to npm still)

@es128
Copy link
Contributor

es128 commented Dec 4, 2014

From your project dir, ought to be as simple as:

cd node_modules/watchify
npm i paulmillr/chokidar

@scamden
Copy link
Contributor

scamden commented Dec 4, 2014

wow that makes me feel dumb. but also grateful :) will check it out

On 4 December 2014 at 15:50, Elan Shanker notifications@github.com wrote:

From your project dir, ought to be as simple as:

cd node_modules/watchify
npm i paulmillr/chokidar


Reply to this email directly or view it on GitHub
#31 (comment).

Sterling Camden
Software Engineer
502 Emerson Street | Palo Alto, CA 94301
303.520.2968 | sterling@relateiq.com

@scamden
Copy link
Contributor

scamden commented Dec 5, 2014

yep it seems to work!

the first time i tried it failed, but i have numerous versions of watchify installed throughout a fairly complex build so i prob installed to the wrong one. once i installed in a simple repo it worked for both internal symlinks and npm linked modules! thank you!

@scamden
Copy link
Contributor

scamden commented Dec 12, 2014

@es128 spoke a little soon, the current impl doesn't work for npm links,

the reason it was working for me was because i hadn't linked the module locally, so i was requiring the module from the global space where it was linked. watchedParent() returns true for the symlink to the global space if it is npm linked locally but the fs events instance created for its parent does not receive any change for changes to the files in the linked directory. maybe this is because its a symlink to another symlink?

@scamden
Copy link
Contributor

scamden commented Dec 12, 2014

it looks like if the watched parent directory is not an actual parent of the symlinked files they don't get changes, so linking to something up the directory tree, or to a sibling doesn't get changes. i'm not at all confident that i understand all that's going on there, but maybe
if (!sysPath.resolve(watchPath).indexOf(sysPath.resolve(watchedPath))) { watchPath = watchedPath; return true; }

should check the realPath rather then watchPath?

@scamden
Copy link
Contributor

scamden commented Dec 12, 2014

trying it that does fix the issue, but i imagine you want to create fewer watchers, while this creates one for every file not inside the package directory

@scamden
Copy link
Contributor

scamden commented Dec 12, 2014

@es128 here's the proposed change so you can see it more clearly at least:

#196

@es128
Copy link
Contributor

es128 commented Dec 13, 2014

@scamden Good catch. I believe the underlying cause here is that npm link causes a symlink to a symlink, and by using fs.readlink it's not resolving all the way through to the actual path.

I'm going to double-check my assumptions, but if I'm right then I'll be closing your PR as the better solution will be to replace every instance of fs.readlink with fs.realpath.

The good news is this can be a patch release, so it won't require any further coordination with watchify to get it corrected there.

@es128
Copy link
Contributor

es128 commented Dec 13, 2014

nm that's not what fs.realpath actually does... will need to fs.readlink recursively I guess

@es128
Copy link
Contributor

es128 commented Dec 13, 2014

wait... yes it does... ok I'm going to stop dumping my stream of consciousness into these comments... will let you know when I'm done fixing it

@scamden
Copy link
Contributor

scamden commented Dec 13, 2014

Haha no worries I always feel like him doing that. Just so you know I found
that any symlink outside of my package's folder had this behavior not just
npm link ones.
On Fri, Dec 12, 2014 at 6:32 PM Elan Shanker notifications@github.com
wrote:

wait... yes it does... ok I'm going to stop dumping my stream of
consciousness into these comments... will let you know when I'm doing
fixing it


Reply to this email directly or view it on GitHub
#31 (comment).

@scamden
Copy link
Contributor

scamden commented Dec 13, 2014

So I was probably wrong about my initial thought that it was the double
symlink. It looks to me like the realpath passed into the watch function is
correct but the path which gets turned into the watchpath is wrong, and can
push a clearer repro in a few hours if it helps
On Fri, Dec 12, 2014 at 7:17 PM Sterling Camden sterling@relateiq.com
wrote:

Haha no worries I always feel like him doing that. Just so you know I
found that any symlink outside of my package's folder had this behavior not
just npm link ones.
On Fri, Dec 12, 2014 at 6:32 PM Elan Shanker notifications@github.com
wrote:

wait... yes it does... ok I'm going to stop dumping my stream of
consciousness into these comments... will let you know when I'm doing
fixing it


Reply to this email directly or view it on GitHub
#31 (comment).

@es128
Copy link
Contributor

es128 commented Dec 13, 2014

I'm having a very hard time setting up a unit test for that that fails with readlink, but passes with realpath. But I can confirm with manual testing that 2710482 resolves the npm link issue I was able to reproduce.

I'll wait for further feedback from you if you can show how to reproduce any other problems.

@scamden
Copy link
Contributor

scamden commented Dec 17, 2014

took at stab at setting up a test today, but can't get it to pass, will try
again tomorrow

On 12 December 2014 at 19:33, Elan Shanker notifications@github.com wrote:

I'm having a very hard time setting up a unit test for that that fails
with readlink, but passes with realpath. But I can confirm with manual
testing that 2710482
2710482
resolves the npm link issue I was able to reproduce.

I'll wait for further feedback from you if you can show how to reproduce
any other problems.


Reply to this email directly or view it on GitHub
#31 (comment).

Sterling Camden
Software Engineer
502 Emerson Street | Palo Alto, CA 94301
303.520.2968 | sterling@relateiq.com

@scamden
Copy link
Contributor

scamden commented Dec 17, 2014

@es128 ok i finally wrote a test that fails before my patch and passes after. the test only fails when using fsevents. it seems the tests don't run with fsevents true on mac, so the only way i could make the test fail was to override options to useFsEvents = true

watchify uses fsevents in it's version of chokdar because it passes {persistent: true} and in that version of chokidar (0.10.9) it does if (undef('useFsEvents')) opts.useFsEvents = !opts.usePolling;

i need your guidance on how to proceed. the test is only meaningful when run with useFsEvents, but that condition isn't tested by default on mac. should i just leave in the override? or do you even want this test?

@es128
Copy link
Contributor

es128 commented Dec 17, 2014

fsevents mode does get tested: https://github.com/paulmillr/chokidar/blob/master/test.js#L34-L36

What happens when you run the tests that makes you think it is not testing fsevents mode?

@es128
Copy link
Contributor

es128 commented Dec 17, 2014

You could just open the PR and we can collaborate on it from there

@scamden
Copy link
Contributor

scamden commented Dec 17, 2014

oh i see what happened here. its a bug in mocha's it.only impl. I use
jasmine, so found only as a replacement for iit. when using only, it only
runs the last one declared (as opposed to the three you declare)

On 17 December 2014 at 12:53, Elan Shanker notifications@github.com wrote:

fsevents mode does get tested:
https://github.com/paulmillr/chokidar/blob/master/test.js#L34-L36

What happens when you run the tests that makes you think it is not testing
fsevents mode?


Reply to this email directly or view it on GitHub
#31 (comment).

Sterling Camden
Software Engineer
502 Emerson Street | Palo Alto, CA 94301
303.520.2968 | sterling@relateiq.com

@scamden
Copy link
Contributor

scamden commented Dec 17, 2014

kk here ya go:

#199

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants