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

Wait for config file to load before prompting to restart in config.onDidChange callback #19354

Merged
merged 1 commit into from May 17, 2019

Conversation

Projects
None yet
2 participants
@nathansobo
Copy link
Contributor

commented May 17, 2019

Fixes #19323.

In #19246, we stopped awaiting the watching of the config file on Windows for performance reasons. However, directly after the await of the promise returned by watch, we call this.config.onDidChange with the assumption that it has already been loaded. This PR moves those observations into a then callback chained onto the original promise so that we only observe changes once the file is loaded, regardless of whether we call await. It also adds the returned disposable in the then callback on all platforms just to unify the logic as much as possible.

cc @Arcanemagus

@nathansobo nathansobo requested a review from rafeca May 17, 2019

@nathansobo nathansobo changed the title Wait for config file to load before watching and prompting to restart Wait for config file to load beforeprompting to restart in config.onDidChange callback May 17, 2019

@nathansobo nathansobo changed the title Wait for config file to load beforeprompting to restart in config.onDidChange callback Wait for config file to load before prompting to restart in config.onDidChange callback May 17, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Thanks for fixing this @nathansobo and apologies for introducing this issue 🤡

@rafeca

rafeca approved these changes May 17, 2019

@nathansobo nathansobo merged commit 9ced07e into master May 17, 2019

2 checks passed

Atom Pull Requests #20190517.5 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nathansobo nathansobo deleted the ns/fix-restart-prompt branch May 17, 2019

@nathansobo

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

apologies for introducing this issue

No worries. I pinged you just to promote awareness but definitely not to lay blame. Easy mistake to make and a small regression.

nathansobo added a commit that referenced this pull request May 17, 2019

Merge pull request #19354 from atom/ns/fix-restart-prompt
Wait for config file to load before prompting to restart in config.onDidChange callback
@nathansobo

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

Cherry-picked the merge commit to 1.38-releases, so we'll need to issue a release at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.