-
Notifications
You must be signed in to change notification settings - Fork 12.8k
The Incremental API aka "The Greedy File Watcher" #31048
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
Comments
@sheetalkamat, thoughts? We did chat with VS Code a bit about this. Did they see improvements? |
Hey @DanielRosenwasser / @sheetalkamat, I was just wondering if there's been any discussion around this? I can certainly testify to lots of people who use create-react-app / fork-ts-checker-webpack-plugin logging issues related to this. It would be great to know if this is something you're looking to address?
I've blogged about it in the meantime: https://blog.johnnyreilly.com/2019/05/typescript-and-high-cpu-usage-watch.html Thanks for all your excellent work! |
I remember chatting with Sheetal about this in person, but it's been a couple of weeks. My understanding is that this really is something that can only be determined by the end-user because different setups can affect this (e.g. operating system, file system, remote file system, etc.) Is that accurate @sheetalkamat? @RyanCavanaugh? |
Would it be possible for you to share the reasoning behind why the current default is the one used? I ask as, to quote @NeKJ:
NodeJS doc:
He suggests that the present default is a choice that node themselves advocate against using. Also, there seems to a good number of people who are finding the present default problematic. Each day that passes seems to bring another user either voting up the existing issue or commenting 😄 Also, those people who have tried setting the environment variable to It would great to understand the rationale for the current choice over one of the alternatives. Oh and just to add a bit more context, here's some investigation which @NeKJ performed:
|
So my only qualification here is that I wrote the initial implementation of The problem that came up is that last part:
To be more specific, you'll find this under the docs for
So back then we just always went with |
I'm no expert either to be honest. Obviously it's a real subset of people that have come back with positive feedback. It would be awesome if someone with more node knowledge in the watch space could share more information so it would be clear if making a change is unwise. I wanted to make a bit of noise about this as I'm certainly hearing it in the various projects I'm involved with. 😄 I see what you mean about the caveats ... There is more detail: https://nodejs.org/docs/latest/api/fs.html#fs_availability
It sounds from this that most TypeScript users would probably have this available. If I read this right it sounds like it's only some *nix distros where it's not available? Not quite sure what
... means? |
Few corner cases, like when you have docker container with webpack running and the source files are mounted in a volume. Or you have source files mounted via network share (NFS/SMB/WEBDAV). |
Thanks @mk0x9 - that's helpful! If these are as niche as it sounds I feel it's worth considering changing the default. People will always be able to change it if there's problems; and by the sounds of it that may be a problem that never arises anyway.... |
@johnnyreilly We use |
Thanks for the detailed explanation @sheetalkamat. So is it the plan that this behaviour won't be changed? I'm assuming the answer to that question is "yes" - but I want to be sure. 😁 Users of both fork-ts-checker-webpack-plugin and create-react-app app have expressed interest in this issue. @NeKJ has raised a PR which would change the default behaviour using the environment variable: TypeStrong/fork-ts-checker-webpack-plugin#256 I haven't merged the PR as I thought it would make more sense for this change to be made in TypeScript itself. It sounds like that's not going to happen and for good reasons. From what you've said, it sounds like you'd recommend not merging that PR. Is that fair? It sounds like if we did we'd potentially solve one problem and create another. You may be saving us from making a mistake 🤗 I'm thinking about just documenting this clearly in order that people will know the configuration is possible. But here be dragons 🐉 and all that 😁 |
@johnnyreilly We document this in https://github.com/microsoft/TypeScript-Handbook/blob/master/pages/Configuring%20Watch.md#background |
if the default doesn't get changed, any chance we get this as a tsconfig option instead of just an env variable? |
I really like @OneCyrus suggestion - the environment variable approach works but the developer experience isn't great; friction is high. I think making this part of Also, it being part of the |
I hope I'm not adding noise to this issue, but I've discovered something odd which seems to relate to the choice of When you This is concerning because I'm now wondering how many wasted recompiles are happening during advanced git operations, like rebasing. I was curious whether this was a const fs = require('fs');
fs.watch(__dirname + '/main.ts', () => {
console.log('watch');
}); If you run that script and OTOH, if you use fs.watchFile(__dirname + '/main.ts', () => {
console.log('watchFile');
}); Is this a known bug? Is there a recommended workaround? |
Sounds like an issue in the node; IIRC fs.watch* is a platform dependent. Would be interesting to test this issue on the different platforms and compare fs.Stats objects in the callback. |
I would rather think about introducing it in |
@sheetalkamat Is it possible to re-open this issue with an option to set the tsconfig.json? |
@johnnyreilly thank you so much you saved my day I use create react app, this line helped me to fix CPU 100% problm |
@DanielRosenwasser / @sheetalkamat - please, would you consider @OneCyrus suggestion? Or/and could you add some information about this issue to documentation and help to reach more developers? Finding that my CPU load is connected with TS and solving it took many hours of my life + my company IT Support time. Many people have the same problem. It still occurs with TypeScript 3.7.2 |
@DanielRosenwasser @sheetalkamat Would you consider switching to Chokidar? It's a battle tested solution used very widely throughout the Node ecosystem. We're in the process of switching to TypeScript at work, and the watch processes are taking ~45% cpu each on my 2015 MacBook Pro. As far as I am aware Node's built-in file watching capabilities are generally considered broken and not suitable for production use. |
@nicoburns I'm not sure this is true anymore. Webpack 5 have just moved from Chokidar to Node's native fs system
I don't think they would have made that change if it was broken. |
Heya!
Maintainer of the fork-ts-checker-webpack-plugin here. Under the covers, the plugin uses the TypeScript incremental API. Various issues have been raised when using this with the level of CPU usage on idle; i.e. it's high!
The marvellous @NeKJ did some digging and discovered that this could be remedied by setting an environment variable:
TSC_WATCHFILE=UseFsEventsWithFallbackDynamicPolling
.This changes the behaviour of TypeScripts approach to file watching in a highly desirable fashion. You can see details of @NeKJ's investigation here:
TypeStrong/fork-ts-checker-webpack-plugin#256
As you'll see, this is a PR for
fork-ts-checker-webpack-plugin
that changes theTSC_WATCHFILE
environment variable. If we merge it the users offork-ts-checker-webpack-plugin
should gain a better experience on the resources front.However, it feels like this is potentially the wrong place to make this change. So before we merge and ship this I wanted to throw it back to you. If this affects users of
fork-ts-checker-webpack-plugin
then I'd hazard a guess other people who use the API are being burned too. Changing the default infork-ts-checker-webpack-plugin
won't help them.Should the default file watching approach used by TypeScript be switched to
TSC_WATCHFILE=UseFsEventsWithFallbackDynamicPolling
or similar? So all users of this API have a better experience?The text was updated successfully, but these errors were encountered: