Change file watching back to polling.

[zhengbli]

We tried the non-polling solution for file watching since last year, and it kept causing us a number of problems regarding watcher instance count limit, cross-platform compatibility, file locking, network file system compatibility etc; while the gain wasn't quite significant to justify all the efforts spent fixing these issues. Therefore it may be better to just go back to polling, which worked reliably for us before and don't have these problems.

[basarat]

My experience with file watching has been similar : https://github.com/TypeStrong/atom-typescript/blob/master/docs/faq.md#atom-typescript-is-complaining-about-not-finding-files-or-other-weird-errors

However with alm tools I am going with a dedicated process to do the disk watching (design) and it hasn't failed me on my system. Of course it might start to be a problem as more people use it. An additional of-course is that I offload the logic to https://github.com/paulmillr/chokidar since I have the luxury to use any npm package I want because package size isn't a concern for me yet 🌹

[myitcv]

Would this revert to polling for all platforms? If so, then I would be against this change. I don't think it needs be a black-or-white solution; if platforms support a fast non-polling approach we should take advantage of that rather than revert to the lowest common denominator.

To that end, is there merit in having this be option-driven until such a time as the platform-support detection is refined?

--watch      # non-polling
--watchPoll  # polling

Or simply utilise some external library/binary that does this well already - have Watchman et al not come up with solutions/compromises already?

[zhengbli]

@basarat Thanks for sharing your experience! We considered chokidar too, but in the end we don't want to add the first dependency to the repo, and they inherited some of the limitation of node file watching too (lock parent folder for example). Plus the package size concern as you described.

@myitcv having different implementation for different systems means more complex logic and more issues to maintain. And sometimes differentiating by OS doesn't help, for example in cases like mounted network folders, or folders shared between virtualbox host and guest OSes, which are hard to detect in the first place because node only tell us that much. As far as I know, some of the problems we encountered are also problems for Watchman and Chokidar (see #7420 and paulmillr/chokidar#446), we need to have stronger reasons to add the first dependency for the repo. We didn't have any of these problems when using polling.

In terms of the option-driven route, we still need to decide the default behavior. If we set it to polling, which is not likely to cause many issues and therefore will become the majority of cases anyway. In addition, it may lead to new issues (imagine if the default mode is polling and you have a non-polling project open, and then you deleted the tsconfig.json so your opened files become loose files, then many things need to happen to make it right). So I lean towards just to keep the implementation simple for now, and in the future maybe hand the task off to editors, or add the non-polling option when node's native file watching becomes more mature.

[myitcv]

@zhengbli thanks for the response. Agree it is indeed a case of striking a balance.

having different implementation for different systems means more complex logic and more issues to maintain

Indeed, but if you only provide a polling option you don't eliminate all issues (see below)

And sometimes differentiating by OS doesn't help, for example in cases like mounted network folders, or folders shared between virtualbox host and guest OSes, which are hard to detect in the first place because node only tell us that much

Indeed, which is why having the option to switch is critical (this is what I meant by my comment of the auto-detection logic not ever being perfect)

we need to have stronger reasons to add the first dependency for the repo

Totally your call, I was simply pointing out that you end up solving the same problems, suffering from the same issues etc.

We didn't have any of these problems when using polling.

No but you did have this problem - and I don't think that goes away... unless I'm mistaken?

In terms of the option-driven route, we still need to decide the default behavior. If we set it to polling, which is not likely to cause many issues and therefore will become the majority of cases anyway

I guess it depends how many projects are large projects, where large is defined in terms of the number of files sufficient to trigger the aforementioned problem

in the future maybe hand the task off to editors

... or some other process. You could even define an API by which another process can "poke" the compiler.

non-polling option when node's native file watching becomes more mature.

I guess my objection to this is that you are taking away the non-polling option until it becomes "more mature" - we don't have a clear idea of when that will be.

Can you not simply put the non-polling option behind a flag for now, in lieu of any other non-polling solution?

[mhegazy]

👍

[zhengbli]

@myitcv The problem you mentioned would be there after this change. We used to have different polling implementation for tsc and tsserver, then we unified them for the sake of simplicity, which caused the slowness for tsc if polling was used. With simply fs.watchFile, change detection should be fast, though CPU consumption may be a little higher.

We can keep both and add an environment variable to switch to non-polling for tsc. I'll update soon.

[myitcv]

We can keep both and add an environment variable to switch to non-polling for tsc. I'll update soon.

@zhengbli - thanks

[fedu]

Should this be fixed in v2.3.4? I'm getting npm install errors when VS Code is open. Always need to close it.