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

Have problems with promisified chrome.contextMenus.create method #23

Closed
q00n opened this issue Nov 12, 2019 · 2 comments · Fixed by #26 · May be fixed by #27
Closed

Have problems with promisified chrome.contextMenus.create method #23

q00n opened this issue Nov 12, 2019 · 2 comments · Fixed by #26 · May be fixed by #27
Assignees
Labels

Comments

@q00n
Copy link

q00n commented Nov 12, 2019

Hi, promisified chrome.contextMenus.create method doesn't returns id of the created menu

@KeithHenry
Copy link
Owner

@q00n well spotted - that method has a slightly different pattern to most. The common pattern (in TS syntax) looks something like action(...params, callback: result: T => void): void and we turn it in to action(...params): Promise<T>.

However, chrome.contextMenus.create is a little different as it immediately returns the created menu, more like create(createProperties, callback: result: () => void): string | number, and this gets turned into action(createProperties): Promise<void>.

I think this method (and any others where the callback doesn't get passed the result) need a different promisify function.

@KeithHenry KeithHenry self-assigned this Dec 2, 2019
@KeithHenry KeithHenry added the bug label Dec 2, 2019
dlh3 added a commit to dlh3/chromeExtensionAsync that referenced this issue Sep 22, 2020
The await keyword will work fine with the `create` function's return (without a Promise), so by skipping the Promisification of this function, it will work in more cases, namely when used with async/await.  

Unfortunately, this won't work with promise chaining (using `then()`), but that can be managed by wrapping the function with `Promise.resolve()`.  I have another idea about how that could also be tackled...  I'll open a separate PR in a bit.

Fixes KeithHenry#23.
dlh3 added a commit to dlh3/chromeExtensionAsync that referenced this issue Sep 22, 2020
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.
@dlh3
Copy link
Contributor

dlh3 commented Sep 22, 2020

This is pretty old now, but as @KeithHenry pointed out, it seems that chrome.contextMenus.create accepts a callback, but it isn't actually an asynchronous action (since it returns immediately). As an aside, I'd love to know how this inconsistency came about; I suspect it was an intern and an overworked lead reviewing the PR, but who knows 🤷🏻‍♂️.

While this project could absolutely implement a completely separate promisify function, I had a couple ideas for working with the existing one:

  1. Remove non-standard contextMenus.create #26 was my initial solution. By simply removing the offending method(s) from the applyMaps argument, the function return wouldn't be suppressed by the promise wrapper. This would work fine for async/await constructs, but would fail for .then() or any other uses of the Promise API, since there would be no promise returned. Of course, the developer could easily wrap the call in Promise.resolve(...). This solution would also be less durable, since any other inconsistencies would have to be identified and removed from the applyMaps argument.

  2. To address the pitfalls of Remove non-standard contextMenus.create #26, I arrived at alternative solution Handle edge case with no callbackArgs #27. This one is more consistent, since it still wraps the chrome.contextMenus.create function in a promise. It's also (hopefully) more durable, since it would automatically apply in the cases where the target function's callback provided no arguments. In this implementation, when callbackArgs is undefined or zero-length, we resolve the promise with the value returned by the function itself.

I recommend #27. 🙂

dlh3 added a commit to dlh3/chromeExtensionAsync that referenced this issue Sep 29, 2020
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.
dlh3 added a commit to dlh3/chromeExtensionAsync that referenced this issue Sep 29, 2020
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants