Make file watching compatible with node version 6.x (runs on 0.10 too) #12647

zaggino · 2016-08-03T23:53:19Z

trying to make brackets work with the new version of node in the shell

cc @ficristo (I couldn't commit to your repo that's why I merged your branch here)

ingorichter · 2016-08-04T05:47:19Z

Okay, I'm able to build on OSX. Brackets launches and changes in the opened folder will show up in the project tree. Adding a file in the finder will update the project tree. The same applies for deleting a file. The whole thing stops working, the moment I open a different folder/project. From that point on I don't receive any change notifications anymore and I'm unable to open a different folder/project.

zaggino · 2016-08-04T06:10:12Z

Thanks @ingorichter , same thing happening on Windows, I'll be looking into it when I'll have some free time.

zaggino · 2016-08-05T00:13:36Z

@ingorichter opening a different project/folder should be fixed now.

zaggino · 2016-08-05T02:50:54Z

Good news, all my testing is passing and I can't see any more problems. Now the question: chokidar allows pushing ignore property to the library itself to ignore some entries, which is great.

The way it currently works: brackets-web get all file change notifications (even from .git folders, and that's a heap of notifications) from brackets-shell and these are filtered on the web side.

The way it could work: brackets-web could pass filter to the brackes-shell to use with chokidar and these would be filtered on shell side which would be great (much less change events and load on the domain connection) but it would also mean that the api between web and shell would have to change. This would be concern for other brackets-web implementations which doesn't use brackets-shell.

Question is: make this breaking change in this PR or not?

@ingorichter @ficristo @petetnt @busykai @abose @marcelgerber

ingorichter · 2016-08-07T06:32:16Z

👏 @zaggino. That works way better now. Thanks for resolving this issue.
I like the idea of having everything filtered on the shell side. Perhaps we can have a feature flag to have both available. At least for some time before dropping the current implementation. It will be a great improvement over the existing implementation and should improve the performance (at least a bit). Perhaps we could even get rid of the TOO_MANY_FILES limitation.

petetnt · 2016-08-08T05:32:53Z

I am with @ingorichter with having it behind a feature flag but enabled by default. It brings huge benefits for Brackets.

Great job with this PR and the current progress with the Node update too in general, really excited about this one 👍

zaggino · 2016-08-16T05:32:33Z

@ingorichter @petetnt implemented & tested

I've actually been using this branch and related shell build daily for a few days now "in production" and I haven't encountered any side effects.

Considering this finished, now it needs a review and hopefully a merge soon.

petetnt · 2016-08-16T05:40:43Z

Great stuff @zaggino, I'll try this as an daily driver today and try to review the code side asap

zaggino · 2016-08-18T00:41:06Z

Excluding node_modules too, watching over them with chokidar causes problems for npm installations and updates to run in a project folder (ref npm/npm#10826 (comment)). It'd be highly unusual workflow working inside a node_modules folder anyway.

ingorichter · 2016-08-18T04:37:20Z

Okay, I'm testing it now and report back when I see things behaving weird.

ficristo · 2016-08-18T06:19:54Z

Excluding node_modules

I really hope we'll make these user configurable (followup is fine). I think, for example, we should ignore bower_components by default too.

zaggino · 2016-08-18T06:22:45Z

Followup is fine, I prefer not to build more features until this is merged.

ficristo · 2016-08-22T08:34:08Z

src/filesystem/impls/appshell/node/FileWatcherDomain.js

@@ -99,7 +99,8 @@ function watchPath(path, ignored) {
            ignoreInitial: true,
            ignorePermissionErrors: true,
            followSymlinks: true,
-            ignored: ignored
+            ignored: ignored,
+            usePolling: process.platform === "win32"


This is sad, @zaggino why you need this?

There's no fsevents on Windows so polling is required, right? That is unless we continue to use fsevents-win 🐱

sadly, chokidar causes watched files to lock-up, can't use npm install or bower install over a watched directory, will need to investigate more

I just saw there were already some issues filed against chokidar and the solution seems to use the usepolling option 😞
It seems to me that even VSCode uses its own watcher system on Windows: https://github.com/Microsoft/vscode-filewatcher-windows

By the way on unix they seems to use these options: https://github.com/Microsoft/vscode/blob/0d658f2c0308c770b90df5a73816f0c045817689/src/vs/workbench/services/files/node/watcher/unix/chokidarWatcherService.ts#L30

thanks @ficristo , good pointers, I'll update this PR soon

usePolling on windows

add note to a FileSystemStats constructor

forgotten arg name in jsdoc jsdoc comment do a bit of renaming remove unnecessary export refactor FileWatcherDomain

reverted node in travis update coment forgotten export

refactor, fix stats bug

…rite notifications

fix jsdoc rename fwm to FileWatcherManager remove tabs update comment add missed param to jsdoc

zaggino · 2016-08-24T22:10:53Z

@ficristo this should be ready for merging now. I've squashed commits that didn't have much of a meaning on their own like "refactor" or "add comment". Grunt script for installing production node_modules into a dist folder is also present (and tested).

ficristo · 2016-08-25T05:40:20Z

I think we had iterate on this PR enough for merging.
@petetnt @ingorichter if you have found any issues we can look in a followup.

@zaggino amazing work. Thank you.

zaggino added the fix in progress label Aug 3, 2016

zaggino self-assigned this Aug 3, 2016

zaggino force-pushed the shell-node6 branch 2 times, most recently from b38abeb to 2cd4cdc Compare August 4, 2016 04:59

ficristo mentioned this pull request Aug 4, 2016

[PoC] Use chokidar instead of the fsevents/fsevents_win combo #12190

Closed

zaggino force-pushed the shell-node6 branch from 78ddc82 to 5ee6c14 Compare August 5, 2016 02:13

zaggino mentioned this pull request Aug 5, 2016

[email protected] support not possible until brackets update bundled node version to 4.x brackets-userland/brackets-eslint#36

Closed

zaggino force-pushed the shell-node6 branch from 1e5b0ea to 67dccf9 Compare August 5, 2016 02:59

zaggino added needs review and removed fix in progress labels Aug 16, 2016

zaggino added this to the Release 1.8 milestone Aug 16, 2016

zaggino removed their assignment Aug 16, 2016

This was referenced Aug 16, 2016

Add file tree exclude in core #12417

Closed

Use a filewatcher library #12187

Closed

zaggino force-pushed the shell-node6 branch from 71e4eb5 to 33cf812 Compare August 18, 2016 04:15

ficristo reviewed Aug 22, 2016
View reviewed changes

zaggino added 18 commits August 25, 2016 08:07

exclude node_modules too

65bc4f8

usePolling on windows

added npm-shrinkwrap

c9b9dbb

more readable condition

b54781a

add note to a FileSystemStats constructor

use simplified group of watch events

9a6d16e

explain better

be4c7c8

forgotten arg name in jsdoc jsdoc comment do a bit of renaming remove unnecessary export refactor FileWatcherDomain

add c# file watcher executables from vscode

b431042

add useful options to chokidar

c4a226f

add glob

2b2bfc1

refactoring

48c200e

implement CSharpWatcher

8b7c0b1

reverted node in travis update coment forgotten export

avoid circular dependency errors

23f687e

do not notify about terminations when closing watcher

68fe2f1

optimize: do not call domain function with an empty list

1096f74

ensure _hash is present in file system stats

3cf15ea

refactor, fix stats bug

fix the bug with shell rounding the stats mtime causing false blind w…

9c78e36

…rite notifications

add MS license for CodeHelper.exe

84b5fa7

fix jsdoc rename fwm to FileWatcherManager remove tabs update comment add missed param to jsdoc

add adobe license headers to new files

9077970

add npm install --production script to grunt build

4e17c98

zaggino force-pushed the shell-node6 branch from c50bac9 to 4e17c98 Compare August 24, 2016 22:09

zaggino mentioned this pull request Aug 24, 2016

Update Tern & Acorn #11569

Merged

ficristo merged commit be62881 into master Aug 25, 2016

ficristo deleted the shell-node6 branch August 25, 2016 05:38

ficristo mentioned this pull request Aug 25, 2016

use npm to download extension dependencies after installing from registry #10602

Merged

zaggino mentioned this pull request Aug 26, 2016

update shell node version to 6.3.1 adobe/brackets-shell#543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make file watching compatible with node version 6.x (runs on 0.10 too) #12647

Make file watching compatible with node version 6.x (runs on 0.10 too) #12647

zaggino commented Aug 3, 2016

ingorichter commented Aug 4, 2016

zaggino commented Aug 4, 2016

zaggino commented Aug 5, 2016

zaggino commented Aug 5, 2016

ingorichter commented Aug 7, 2016

petetnt commented Aug 8, 2016

zaggino commented Aug 16, 2016

petetnt commented Aug 16, 2016

zaggino commented Aug 18, 2016

ingorichter commented Aug 18, 2016

ficristo commented Aug 18, 2016

zaggino commented Aug 18, 2016

ficristo Aug 22, 2016

petetnt Aug 22, 2016

zaggino Aug 22, 2016

ficristo Aug 22, 2016

zaggino Aug 22, 2016

zaggino commented Aug 24, 2016

ficristo commented Aug 25, 2016

Make file watching compatible with node version 6.x (runs on 0.10 too) #12647

Make file watching compatible with node version 6.x (runs on 0.10 too) #12647

Conversation

zaggino commented Aug 3, 2016

ingorichter commented Aug 4, 2016

zaggino commented Aug 4, 2016

zaggino commented Aug 5, 2016

zaggino commented Aug 5, 2016

ingorichter commented Aug 7, 2016

petetnt commented Aug 8, 2016

zaggino commented Aug 16, 2016

petetnt commented Aug 16, 2016

zaggino commented Aug 18, 2016

ingorichter commented Aug 18, 2016

ficristo commented Aug 18, 2016

zaggino commented Aug 18, 2016

ficristo Aug 22, 2016

Choose a reason for hiding this comment

petetnt Aug 22, 2016

Choose a reason for hiding this comment

zaggino Aug 22, 2016

Choose a reason for hiding this comment

ficristo Aug 22, 2016

Choose a reason for hiding this comment

zaggino Aug 22, 2016

Choose a reason for hiding this comment

zaggino commented Aug 24, 2016

ficristo commented Aug 25, 2016