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

Rejecting promise in registerSave callback should revert changes to options page #21

Open
tdulcet opened this issue Jun 6, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@tdulcet
Copy link

tdulcet commented Jun 6, 2022

Rejecting the promise in a registerSave callback should automatically revert any changes to the option page.

This line assumes that the promises will always resolve:

const saveTriggerValue = await Trigger.runSaveTrigger(option, optionValue, event);

It produces an "uncaught exception" error if they do not, which leaves the options page in an invalid state:
image
This is particularity an issue when using the PermissionRequest library.

The solution is probably for the library to call preventDefault() whenever the promise is rejected.

@rugk
Copy link
Member

rugk commented Jun 6, 2022

Did not look into the code too deeply, but what you say makes sense. Feel free to create a pr.

Just note initially that was not intended to fail/it could never fail or was expected to.

* @return {Promise|void} optionally, to use await

Also it should probably not revert all changes but only the single option it is called for. In general ootions saving should always work.
Or to make it explicit we could introduce a new symbol.
Though in case of an error, i agree, it kinda makes sense to revert the setting to the currently saved one. (Though such logic can always fail and make stuff complex with custom options triggers etc.)

@rugk rugk added the enhancement New feature or request label Jun 6, 2022
@rugk
Copy link
Member

rugk commented Jun 6, 2022

Also do you have a specific use case for that or is it just an idea in general?

@tdulcet
Copy link
Author

tdulcet commented Jun 6, 2022

Also do you have a specific use case for that or is it just an idea in general?

With the PermissionRequest library, if the user denies the permission request, the promise is rejected, which produces this issue. For example, try enabling the Omnibox option in the Awesome Emoji Picker, but then deny the clipboard write permission. The Omnibox option appears to still be enabled, when it is actually disabled.

Anyway, my specific use case was allowing the user to confirm a potenchally dangerous option change with window.confirm(). See: rugk/unicodify#72

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

No branches or pull requests

2 participants