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

Handle edge case with no callbackArgs #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dlh3
Copy link
Contributor

@dlh3 dlh3 commented Sep 22, 2020

This is an alternative approach to fixing #23. For chrome functions that pass no arguments to their callbacks, we should try to propagate the function's own return value.

Honestly, I'm pretty confused about how it is even able to work. It makes sense, kind of, but I'm surprised that I can access the function return inside an arrow function defined as an inline function argument. In any case, I tested and confirmed that this works.

Fixes #23 and closes #26.

@KeithHenry
Copy link
Owner

@dlh3 I'm not sure I want to make this change - it makes good sense to always get the return value, but I'm not sure there aren't contexts where the call back should have no arguments or where holding on to a return value might be an issue.

I think I'd rather ensure that none of the mapped API functions are expected to return a value direct value.

However, it's a good idea and I want to come back to it when I get some time to test it.

@KeithHenry KeithHenry self-assigned this Sep 29, 2020
@dlh3
Copy link
Contributor Author

dlh3 commented Sep 29, 2020

So, I thought through that.

Under the current implementation:

  1. Any functions with zero-arg callbacks will return a promise which will always resolve to undefined (or reject, if there was an exception or runtime.lastError). As such, no consumers should currently expect a resolved value, and are likely working around this (as would be necessary for Have problems with promisified chrome.contextMenus.create method #23). This PR would only ever "clobber" an undefined value, by instead propagating the function's own return value, which could also be undefined, if there is truly nothing to return.

  2. Any functions with non-zero-arg callbacks will continue to return the value(s) from their callback.

I did think through the consequences and perform quite a bit of testing when I developed this solution, so I have a high level of confidence in this approach. While #26 was a satisfactory – though admittedly hacky – workaround, it only mitigated the issue for a single known function (contextMenus.create) and it would break this library's API for any developers using promise chains (rather than async/await) since that function is no longer promisified.

Thanks @KeithHenry, but I do really hope you find time to revisit this PR. I am confident it is a much better approach than @26.

dlh3 pushed a commit to dlh3/chromeExtensionAsync that referenced this pull request Sep 29, 2020
This reverts commit ab19d0d.  That was introduced in PR KeithHenry#26 as an alternative approach to KeithHenry#27.  However with the more consistent solution of KeithHenry#27, `contextMenus.create` should be promisified, in order to ensure a consistent API (so it returns a Promise, like everything else).
This is an alternative approach to fixing KeithHenry#23.  For chrome functions that pass no arguments to their callbacks, we should try to propagate the function's own return value.

Honestly, I'm pretty confused about how it is even able to work.  It makes sense, kind of, but I'm surprised that I can access the function return inside an arrow function defined as an inline function argument.  In any case, I tested and confirmed that this works.

Fixes KeithHenry#23 and closes KeithHenry#26.
This reverts commit ab19d0d.  That was introduced in PR KeithHenry#26 as an alternative approach to KeithHenry#27.  However with the more consistent solution of KeithHenry#27, `contextMenus.create` should be promisified, in order to ensure a consistent API (so it returns a Promise, like everything else).
@dlh3
Copy link
Contributor Author

dlh3 commented Sep 29, 2020

Updated branch/PR to revert the removal of contextMenus.create (#26), since the goal of #27 is to ensure the API is consistent.

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.

Have problems with promisified chrome.contextMenus.create method
2 participants