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

Fix config reload after vim edit #2718

Merged
merged 2 commits into from
Mar 3, 2018
Merged

Conversation

chabou
Copy link
Collaborator

@chabou chabou commented Feb 27, 2018

When vim edit a file, it write a temp file and move it.
In this case, our watcher didn't work anymore.

This PR rewatch file when file is renamed/mv

app/config.js Outdated
if (eventType === 'rename') {
_watcher.close();
// Ensure that new file has been written
setTimeout(() => setWatcher(), 500);
Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to add retry here? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would be our retry strategy? Only one time? Retry until it works?
If we think that 500ms is not enough, we need to put some try/catch too.

But I really think that it would work 99.99%. If not, maybe we'll add some retries later?

Mmmmh I should try this with config file remove...

Do not merge it please, I think it needs some more tests on some edge cases

@chabou
Copy link
Collaborator Author

chabou commented Mar 3, 2018

I made a lot of test and it works really great.
I added a try/catch but it should be unused. If a config file is missing, a new one is written: https://github.com/zeit/hyper/blob/24b83615d4f37cfead075c5a80383dac9c3311ee/app/config/import.js#L41

If writing failed, an exception will be shown.
Thus, we can silently (but with log) discard an hypothetical watch exception.
User would have to restart its application to have a config watcher, but this is ok for me because it should not happen.

@chabou chabou merged commit bf2d6ea into vercel:canary Mar 3, 2018
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.

2 participants