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

**/*.jsx is matching test.js #420

Closed
jsheely opened this issue Dec 19, 2015 · 19 comments
Closed

**/*.jsx is matching test.js #420

jsheely opened this issue Dec 19, 2015 · 19 comments

Comments

@jsheely
Copy link

jsheely commented Dec 19, 2015

Doing a pattern match for /**/*.jsx and change events are firing for *.js on windows

Where exactly is the pattern match validating the file?

@jsheely
Copy link
Author

jsheely commented Dec 19, 2015

To say in a different way.

filename.js matches for pattern filename.jsx So the change event is actually on filename.jsx

@jsheely
Copy link
Author

jsheely commented Dec 26, 2015

More information. I was able to track this bug down into libuv of node.

See: nodejs/node#4351 and pull request nodejs/node#4429

In order to complete this fix in chokidar we would need to filter the file names coming out of the fs.watch as they will not exactly match the pattern.

At this point if you are watching fs.watch('*/.jsx') a change event for filename.js may come through but unlike before the pull-request at least it will accurately say filename.js instead of mistakenly saying filename.jsx even though that wasn't the file you changed.

@paulmillr
Copy link
Owner

Interesting.

Would you be able to send us a PR that would fix this behavior (w tests)?

@sashashakun
Copy link

I will try to send a PR with fix of this bug. At first I will try to repeat bug on my machine, and then fix. I will be back in one or two days with the results)

@jsheely
Copy link
Author

jsheely commented Feb 28, 2016

Some additional resources on the actual fix inside of libuv libuv/libuv#682

This is mainly an issue with libuv on windows but still may need some defensive code added to chokidar to ensure compatibility.

@sashashakun
Copy link

I am successfully reproduce the error. Now i am trying to found where we get the files that fit the pattern, then i think about adding additional check for fitting the pattern and test for it. What do you think @jsheely ,@paulmillr ,any advices, opinions?

@jsheely
Copy link
Author

jsheely commented Feb 29, 2016

@sashashakun Last I looked into this the issue is that chokidar assumes if a watch event is fired it must be on the right file and does not check what file is being reported as changed. (Since if libuv was working correctly it would have to be the correct file).

I believe the issue starts to cascade from here. https://github.com/paulmillr/chokidar/blob/master/lib/nodefs-handler.js#L37

My patch to libuv will output what file was changed which will not match what is being watched by chokidar but an event will still come through all the same.

I haven't gotten any activity on my pull-request yet, which is just a band-aide to the problem. In an ideal world libuv would never output an event if it doesn't match the wildcard file name.

But if my band-aide is the only viable option then post processing the file being reported will be needed.

@es128
Copy link
Contributor

es128 commented Mar 1, 2016

@sashashakun my advice is to try something like

// fix for Windows bug: https://github.com/nodejs/node/issues/4351
if (process.platform === 'win32' && this._isIgnored(path)) return;

at the top of FSWatcher.prototype._emit

@sashashakun
Copy link

Thank everyone for advices, will try this things

@paulmillr paulmillr added the bug label Mar 2, 2016
@sashashakun
Copy link

Ok, I have some results. At first look, I am agree with @jsheely ,bug in libuv itself, I mean that native node API that used in nodefs-handler#L37:

fs.watch(path, options, handleEvent);

return information that .jsx file was changed, but in fact .js was. So may be we can wait for fix in libuv for this, and add some additional checking after that?

@es128 I made that you said, but it didnt help(

@es128
Copy link
Contributor

es128 commented Mar 2, 2016

Yeah if both files are within the watch paths and the event is being misreported from a lower level then my suggestion would be ineffective (and there likely isn't anything we can do here).

In the original issue report it was suggested that paths are being filtered with a glob pattern like *.jsx, and events were coming out for paths that shouldn't have even been watched. In this case, an approach like I suggested might be effective for suppressing those events, but the side effect would be completely missed events.

So waiting for a fix at libuv is probably the only thing we can do.

@paulmillr
Copy link
Owner

@sashashakun yep, libuv folks should review the code and merge the PR. Other fixes are ineffective.

libuv/libuv#682

@sashashakun
Copy link

Hi all again, checked out conversation here libuv/libuv#682 and also saw this PR libuv/libuv#858
Am I right that it will solve this issue or we need two add some internal checking in chokidar?

(sorry, my C knowledge is very poor)

@jsheely
Copy link
Author

jsheely commented May 15, 2016

Based on the considered changes to libuv/libuv#682 chokidar will have to post process the event to ensure which file was matched meets the fuzzy wild card search.

As it stands right now chokidar assumes that if an event comes out fs.watch then it must have been for one of the files that it setup a watch for.

@es128
Copy link
Contributor

es128 commented May 17, 2016

@jsheely as I described before, following through on your suggestion won't really fix the problem. Either the incorrect event will be filtered out instead of being misreported, so the true event will still be missed, or if both file paths are being watched there will be no change in behavior. There does not appear to be a possible workaround that can be added to chokidar to fix for the libuv bug.

@jsheely
Copy link
Author

jsheely commented May 17, 2016

@es128 Agreed. We have to wait for the libuv bug to be fixed.

For anyone monitoring this issue. The real fix is to just try and not put your jsx and js files in the same folder. If you follow the standard: ./src and ./build folder structure this won't be a problem on windows.

@UltCombo
Copy link

This may be fixed in Node v7.1.0 as it uses libuv 1.10.0 which includes a fix for this issue. Can someone test on a Windows machine to confirm?

@UltCombo
Copy link

I've tested on a Windows machine now, with Node 6.9.1 (reproduces the issue) and Node 7.1.0 (works as expected).

The bug was in libuv, part of the Node.js core. It is fixed as of libuv 1.10.0, which ships with Node 7.1.0.

@es128
Copy link
Contributor

es128 commented Nov 21, 2016

Thanks @UltCombo!

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

No branches or pull requests

5 participants