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.

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

implemented watch mode #167

Merged
merged 3 commits into from Mar 15, 2020
Merged

implemented watch mode #167

merged 3 commits into from Mar 15, 2020

Conversation

KisyaLisya
Copy link
Contributor

@KisyaLisya KisyaLisya commented Mar 13, 2020

I've trying to implement this as simple as I could do it. So it looks like enough for the requirements maybe it's not that good inside, so let's discuss what we can do better :)

packages/size-limit/run.js Show resolved Hide resolved
], {
ignored: '**/node_modules/**'
})
watcher.on('change', async () => {
Copy link
Owner

@ai ai Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user presses Ctrl+S twice very fast, chokidar will start 2 Size Limit’s mainCalc in parallel.

We need a way to throttle it. We can look at a solution in chokidar or copy my solution from print-snapshots.

Copy link
Contributor Author

@KisyaLisya KisyaLisya Mar 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed this. The only one question. Why do you set delay to 200ms? Do you need really wait so long or it's enough to wait next cycle like ( setTimeout(() => ..., 0) )?

Copy link
Owner

@ai ai Mar 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. We need to wait for full circle. print-snapshots wait for full circle and 200ms.

https://github.com/ai/print-snapshots/blob/master/watch.js#L14-L16

@ai
Copy link
Owner

ai commented Mar 13, 2020

Looks good. Only few small things to fix.

@ai ai merged commit 84cb92b into ai:master Mar 15, 2020
1 check was pending
@ai
Copy link
Owner

ai commented Mar 15, 2020

Great work!

Can you remind me of your Twitter account (it will be hard to find in mentions)?

@KisyaLisya
Copy link
Contributor Author

KisyaLisya commented Mar 15, 2020

Great work!

Can you remind me of your Twitter account (it will be hard to find in mentions)?

Oh, thank you! https://twitter.com/your_bulichka

@ai
Copy link
Owner

ai commented Mar 15, 2020

Thanks. Released at 4.4.

Do you want to add another contribution to your GitHub and replace nodemon with your own size-limit --watch in Nano ID?

What name I should use in task at Cult of Martians?

@KisyaLisya
Copy link
Contributor Author

KisyaLisya commented Mar 16, 2020

Yes, I'd like to continue implementing things that helps other developers and even big teams :)

I guess my github nickname, "jayhoney" would be nice

@ai
Copy link
Owner

ai commented Mar 16, 2020

Done https://cultofmartians.com/tasks/size-limit-watch.html#task

I am waiting for PR to Nano ID (you just need to remove nodemon and use size-limit --watch directly)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants