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.

Sign up for GitHub

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

Jump to bottom

[CLOSED] Make file watching compatible with node version 6.x (runs on 0.10 too) #10784

Open
core-ai-bot opened this issue Aug 30, 2021 · 30 comments
Open

[CLOSED] Make file watching compatible with node version 6.x (runs on 0.10 too) #10784

core-ai-bot opened this issue Aug 30, 2021 · 30 comments

Comments

@core-ai-bot
Copy link
Member

Issue by zaggino
Wednesday Aug 03, 2016 at 23:53 GMT
Originally opened as adobe/brackets#12647

related to adobe/brackets-shell#543

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)

zaggino included the following code: https://github.com/adobe/brackets/pull/12647/commits
@core-ai-bot
Copy link
Member Author

Comment by ingorichter
Thursday Aug 04, 2016 at 05:47 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Thursday Aug 04, 2016 at 06:10 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Friday Aug 05, 2016 at 00:13 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Friday Aug 05, 2016 at 02:50 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by ingorichter
Sunday Aug 07, 2016 at 06:32 GMT

👏 @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.

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Monday Aug 08, 2016 at 05:32 GMT

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 👍

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Aug 16, 2016 at 05:32 GMT

@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.

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Tuesday Aug 16, 2016 at 05:40 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Thursday Aug 18, 2016 at 00:41 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by ingorichter
Thursday Aug 18, 2016 at 04:37 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Thursday Aug 18, 2016 at 06:19 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Thursday Aug 18, 2016 at 06:22 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Monday Aug 22, 2016 at 18:26 GMT

I didn't try the PR but I left some comments regardless.
Have you tryed if the non recursive code still works?

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Aug 23, 2016 at 01:15 GMT

Comments addressed, I haven't played with the non recursive stuff as I see little point - the code around recursiveWatch being false wasn't touched in this PR.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Aug 23, 2016 at 03:06 GMT

@ficristo ready for round 2, this now supports Code's CSharpWatcher in win32

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Tuesday Aug 23, 2016 at 07:47 GMT

this now supports Code's CSharpWatcher in win32

Sorry if I wasn't clear... I didn't imply to move to it. But it seems you think it's fine so I trust you.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Aug 23, 2016 at 09:52 GMT

@ficristo It's a good solution to avoid usePolling with chokidar on Windows. Insides probably are similar/same as fsevents_win that Brackets has right now. It's a good, working, tested and proven solution (I never had problems with file watching on Code). I think it'd benefit us to use it and don't have to care about maintaining something of our own.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Tuesday Aug 23, 2016 at 18:36 GMT

I left some other comments.
Before I forget: AFAIK Brackets build scripts do not take in consideration the package dependencies, we should check this.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Aug 23, 2016 at 21:35 GMT

@ficristo done, hope I didn't miss any comments.

npm dependencies PR opened here: adobe/brackets-shell#572

that's all that should be needed, grunt copies everything, even node_modules from adobe/brackets clone: https://github.com/adobe/brackets-shell/blob/master/Gruntfile.js#L177-L184

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Wednesday Aug 24, 2016 at 09:14 GMT

Two things I almost forgotten:

  • before we merge this we should squash the commits.@zaggino you can remove mine: you deserve all the credits!
  • chokidar come with an optional dependecy: fsevents. I suppose we build on OSX before releasing the installer for that platform. Right?

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Wednesday Aug 24, 2016 at 13:58 GMT

@ficristo

a) no commit removing, you laid down a base for this PR and credit is due for that (you also did a good job reviewing it), merge as is

b) you can't build osx shell on non-osx machine so that's right

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Wednesday Aug 24, 2016 at 17:58 GMT

I'm sorry I noticed only now: in newer file you should add the classic license comment of Adobe.
I'm not sure if CSharpWatcher.js needs one

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Wednesday Aug 24, 2016 at 18:04 GMT

I don't think the PR for the build system is right: I'll write more there.
I want this ASAP in the core, so I'm fine to fix the build part in a followup.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Wednesday Aug 24, 2016 at 18:15 GMT

For the commits: I don't pretend you squash everything in a single commit.
But I think commits like these two could be squashed together:
Add 'depth' parameter 8ecb4d72af74e4a0138a4f8e9702993955e9828b
Remove depth, add ignore 5f91ca19cece64afc5780a5db160e6b8fa938f6e
And probably this could be removed:
usePolling on windows e3dbb179026fd9216cdc24db79cbef9d7c74fa68

If you really want to retain all the commits I would like to see better messages.
When I look at the git history, I don't know to what the logs belongs, so I find helpful to see meaningfull logs.
These two messages, for example, don't mean much outside of this PR.
optimize 2b4abcfd308563f1b091b5fb4e1ae243eb7b5bc6
simplify 020f74b0fd2900ea05163462782ce9d8fe5cab37

Sincerely I'm not sure about the policy of Brackets on these things, so it's up to you what to do.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Wednesday Aug 24, 2016 at 18:41 GMT

This time I've run, at least, all the test suite on my Windows 10 PC and it looks very green 🎆

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Wednesday Aug 24, 2016 at 19:00 GMT

Out of curiosity, how did you generate the CodeHelper.exe?

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Wednesday Aug 24, 2016 at 21:33 GMT

Took it from here https://github.com/Microsoft/vscode/blob/314e122b16c5c1ca0288c8006e9c9c3039a51cd7/src/vs/workbench/services/files/node/watcher/win32/CodeHelper.exe. Also I've tried rebuilding file from the sourcecode's in production mode as advised and got the same filesize (83968 bytes).

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Wednesday Aug 24, 2016 at 21:43 GMT

I'm gonna rebase this and rewrite some commit messages, saying that, there's no policy on this and I've never seen anyone really doing that... maybe taking it a bit too far.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Wednesday Aug 24, 2016 at 22:10 GMT

@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).

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Thursday Aug 25, 2016 at 05:40 GMT

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot