search

KeithHenry / chromeExtensionAsync

Promise wrapper for the Chrome extension API so that it can be used with async/await rather than callbacks
MIT License
228 stars 32 forks source link

Handle edge case with no callbackArgs #27

Open dlh3 opened 4 years ago

dlh3 commented 4 years ago

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 commented 4 years ago

@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.

dlh3 commented 4 years ago

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 #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 commented 4 years ago

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