-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Config] Acknowledge when Advanced Settings respond #16485
[Config] Acknowledge when Advanced Settings respond #16485
Conversation
💔 Build Failed |
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.
had a small question
}) | ||
.catch(reason => { | ||
localUpdate(key, initialVal, config.get(key)); | ||
notify.error(reason); | ||
return false; |
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.
is there a way to have this return value covered in a test?
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.
Not without exposing the change
method and allowing the delayedUpdate
method to be overridden, or adding the override to change
and set
, which is undesirable.
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.
I decided it was worth the effort, so I exposed change
as _change
and added tests for true
and false
scenarios.
This adds a return value to the `Promise` returned by `config.set`, which allows callers to `await` it and verify that their setting was applied without checking the settings (which is a race condition).
1c0b7b7
to
78c2a8d
Compare
💔 Build Failed |
💚 Build Succeeded |
I think it might be time to rip out the delayed updater, cc @nreese |
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.
LGTM! Thanks for adding the rejection test
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.
LGTM
This adds a return value to the `Promise` returned by `config.set`, which allows callers to `await` it and verify that their setting was applied without checking the settings (which is a race condition).
6.x/6.3: 1b4fab3 |
This adds a return value to the
Promise
returned byconfig.set
, which allows callers toawait
it and verify that their setting was applied without checking the settings (which is a race condition).A future update might be good to change the
notifier
usage to atoastNotification
, but that is a wider effort.