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
Improve watch mode #238
Improve watch mode #238
Conversation
I'm also not sure how to test it properly yet. But I think existing tests for watch mode are not enough. And these tests actually make you add |
This fix will not work for We can replace |
Is that a case for a library? Meaning it's needed only in cases where webpack is also being used? |
Yeap. |
It would be convenient to use webpack's watch mode in such cases then. Or we could allow a plugin to tell which files to add to watch or something like that. But that complicates things a bit. I'll try to think of some simpler solution. Would it be too bad to extend the interface between a tool and a plugin, if needed? |
We can do it, but it looks like overkill. We do not need 100% accurate method. It is OK to call Size Limit again more often that it was needed. I think it is better to remove |
What if we use the same info for watching, which size-limit/packages/file/index.js Line 50 in 08f8af4
So I suggest to run it like this:
If setting up watcher from scratch every time would be too expensive, chokidar has an API to add or remove files being watched. |
Looks very complicated and add false negative error to Why you do not want to just remove |
Removing Other steps in the comment above are for reapplying changes from |
I think it would make sense to either reread |
Let’s solve one issues step-by-step. Let’s fix |
I pushed changes with only extension removal |
packages/size-limit/run.js
Outdated
@@ -80,7 +80,7 @@ module.exports = async process => { | |||
await calcAndShow() | |||
|
|||
if (hasArg('--watch')) { | |||
let watcher = chokidar.watch(['**/*.js', 'package.json'], { | |||
let watcher = chokidar.watch(['**/*', 'package.json'], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove pavkage.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Should I then submit a separate PR with reloading package.json changes? |
Let's do it in a separate PR |
Hello! Really cool project!
But I think the watch mode needs some improvements. Right now it watches all js files in the current directory, excluding node_modules. So now it doesn't work for css, for example. And I think you can imagine how many unnecessary reruns there can be. So I made it watch all the files from config.checks. Not sure if using flatMap is ok for supported node versions though. I can replace it if needed.
Also, despite package.json being watched, changes there are not applied, so I removed it for now to avoid confusion. I'm going to make it apply package.json changes as a next step and I have some more improvements in my mind related to watch mode.
I'm just not sure if it's ok to do in a single PR or multiple. What do you think?