Issue 30024555: Issue 7334 - Remove support for callbacks in API wrappers (Closed)
|
Created:
March 6, 2019, 10:56 a.m. by Manish Jethani Modified:
April 25, 2019, 9:12 a.m. Reviewers:
Sebastian Noack Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
Patch Set 1 : #Patch Set 2 : Rebase #Patch Set 3 : Handle calls with no arguments #
Total comments: 7
Patch Set 4 : Remove checks #MessagesTotal messages: 13
LGTM. At the time we couldn't land it because of the adblockplusui still relying on that behavior, but it seems that change [1] made it already in the last dependency update. Please correct me if wrong? [1]: https://hg.adblockplus.org/adblockplusui/rev/416c962e2725
On 2019/04/24 09:28:36, Sebastian Noack wrote: > LGTM. > > At the time we couldn't land it because of the adblockplusui still relying on > that behavior, but it seems that change [1] made it already in the last > dependency update. Please correct me if wrong? > > [1]: https://hg.adblockplus.org/adblockplusui/rev/416c962e2725 Yes, this seems right.
Patch Set 2: Rebase Patch Set 3: Handle calls with no arguments https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js#newcode120 polyfill.js:120: lastArgumentType == "undefined" && args.length > 0) We need to support calls like `browser.runtime.openOptionsPage()`.
https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js#newcode120 polyfill.js:120: lastArgumentType == "undefined" && args.length > 0) On 2019/04/24 14:42:23, Manish Jethani wrote: > We need to support calls like `browser.runtime.openOptionsPage()`. I just tested on Firefox (where the promise-based API is supported natively) without the polyfill, and browser.runtime.openOptionsPage(undefined) just works fine. So we shouldn't emulate an error in that case.
https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js#newcode120 polyfill.js:120: lastArgumentType == "undefined" && args.length > 0) On 2019/04/24 19:50:38, Sebastian Noack wrote: > On 2019/04/24 14:42:23, Manish Jethani wrote: > > We need to support calls like `browser.runtime.openOptionsPage()`. > > > I just tested on Firefox (where the promise-based API is supported natively) > without the polyfill, and browser.runtime.openOptionsPage(undefined) just works > fine. So we shouldn't emulate an error in that case. But we're not? Quite the opposite, the additional change I made is about letting `browser.runtime.openOptionsPage()` work. Did I miss something in the rebase?
https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js#newcode120 polyfill.js:120: lastArgumentType == "undefined" && args.length > 0) On 2019/04/24 20:09:47, Manish Jethani wrote: > On 2019/04/24 19:50:38, Sebastian Noack wrote: > > On 2019/04/24 14:42:23, Manish Jethani wrote: > > > We need to support calls like `browser.runtime.openOptionsPage()`. > > > > > > I just tested on Firefox (where the promise-based API is supported natively) > > without the polyfill, and browser.runtime.openOptionsPage(undefined) just > works > > fine. So we shouldn't emulate an error in that case. > > But we're not? Quite the opposite, the additional change I made is about letting > `browser.runtime.openOptionsPage()` work. Did I miss something in the rebase? Like I read the code here, you throw if the last argument is undefined given explicitly, Firefox doesn't. Do I miss something?
https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js#newcode120 polyfill.js:120: lastArgumentType == "undefined" && args.length > 0) On 2019/04/24 20:42:29, Sebastian Noack wrote: > On 2019/04/24 20:09:47, Manish Jethani wrote: > > On 2019/04/24 19:50:38, Sebastian Noack wrote: > > > On 2019/04/24 14:42:23, Manish Jethani wrote: > > > > We need to support calls like `browser.runtime.openOptionsPage()`. > > > > > > > > > I just tested on Firefox (where the promise-based API is supported natively) > > > without the polyfill, and browser.runtime.openOptionsPage(undefined) just > > works > > > fine. So we shouldn't emulate an error in that case. > > > > But we're not? Quite the opposite, the additional change I made is about > letting > > `browser.runtime.openOptionsPage()` work. Did I miss something in the rebase? > > Like I read the code here, you throw if the last argument is undefined given > explicitly, Firefox doesn't. Do I miss something? Are you suggesting we don't throw the error at all? My original idea was to do this so we catch mistakes where we're still using callbacks unknowingly. Of course this can cause trouble when you have something like this: function openOptions(callback) { return browser.runtime.openOptionsPage(callback); } openOptions(); It will throw. But that is exactly the intention here. If it throws, we need to migrate that code to use promises. After a couple of releases we can remove the check for `"undefined"` and finally we remove it for `"function"` as well.
https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js#newcode120 polyfill.js:120: lastArgumentType == "undefined" && args.length > 0) On 2019/04/25 08:04:54, Manish Jethani wrote: > On 2019/04/24 20:42:29, Sebastian Noack wrote: > > On 2019/04/24 20:09:47, Manish Jethani wrote: > > > On 2019/04/24 19:50:38, Sebastian Noack wrote: > > > > On 2019/04/24 14:42:23, Manish Jethani wrote: > > > > > We need to support calls like `browser.runtime.openOptionsPage()`. > > > > > > > > > > > > I just tested on Firefox (where the promise-based API is supported > natively) > > > > without the polyfill, and browser.runtime.openOptionsPage(undefined) just > > > works > > > > fine. So we shouldn't emulate an error in that case. > > > > > > But we're not? Quite the opposite, the additional change I made is about > > letting > > > `browser.runtime.openOptionsPage()` work. Did I miss something in the > rebase? > > > > Like I read the code here, you throw if the last argument is undefined given > > explicitly, Firefox doesn't. Do I miss something? > > Are you suggesting we don't throw the error at all? > > My original idea was to do this so we catch mistakes where we're still using > callbacks unknowingly. Of course this can cause trouble when you have something > like this: > > function openOptions(callback) > { > return browser.runtime.openOptionsPage(callback); > } > > openOptions(); > > It will throw. But that is exactly the intention here. If it throws, we need to > migrate that code to use promises. After a couple of releases we can remove the > check for `"undefined"` and finally we remove it for `"function"` as well. I think a polyfill should behave like the implementation it is imitating. I see your point, but once we drop the polyfill and assuming Chrome by then will behave like Firefox, we won't have that feature anymore anyway.
https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js#newcode120 polyfill.js:120: lastArgumentType == "undefined" && args.length > 0) On 2019/04/25 08:18:21, Sebastian Noack wrote: > On 2019/04/25 08:04:54, Manish Jethani wrote: > > On 2019/04/24 20:42:29, Sebastian Noack wrote: > > > On 2019/04/24 20:09:47, Manish Jethani wrote: > > > > On 2019/04/24 19:50:38, Sebastian Noack wrote: > > > > > On 2019/04/24 14:42:23, Manish Jethani wrote: > > > > > > We need to support calls like `browser.runtime.openOptionsPage()`. > > > > > > > > > > > > > > > I just tested on Firefox (where the promise-based API is supported > > natively) > > > > > without the polyfill, and browser.runtime.openOptionsPage(undefined) > just > > > > works > > > > > fine. So we shouldn't emulate an error in that case. > > > > > > > > But we're not? Quite the opposite, the additional change I made is about > > > letting > > > > `browser.runtime.openOptionsPage()` work. Did I miss something in the > > rebase? > > > > > > Like I read the code here, you throw if the last argument is undefined given > > > explicitly, Firefox doesn't. Do I miss something? > > > > Are you suggesting we don't throw the error at all? > > > > My original idea was to do this so we catch mistakes where we're still using > > callbacks unknowingly. Of course this can cause trouble when you have > something > > like this: > > > > function openOptions(callback) > > { > > return browser.runtime.openOptionsPage(callback); > > } > > > > openOptions(); > > > > It will throw. But that is exactly the intention here. If it throws, we need > to > > migrate that code to use promises. After a couple of releases we can remove > the > > check for `"undefined"` and finally we remove it for `"function"` as well. > > I think a polyfill should behave like the implementation it is imitating. I see > your point, but once we drop the polyfill and assuming Chrome by then will > behave like Firefox, we won't have that feature anymore anyway. Fair enough, in that case we have two options: 1. Check only for `"function"` 2. Remove any checks I don't think we should have the checks at all if we're sure we've migrated to promises.
Patch Set 4: Remove checks
On 2019/04/25 08:51:20, Manish Jethani wrote: > https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js > File polyfill.js (right): > > https://codereview.adblockplus.org/30024555/diff/30049559/polyfill.js#newcode120 > polyfill.js:120: lastArgumentType == "undefined" && args.length > 0) > On 2019/04/25 08:18:21, Sebastian Noack wrote: > > On 2019/04/25 08:04:54, Manish Jethani wrote: > > > On 2019/04/24 20:42:29, Sebastian Noack wrote: > > > > On 2019/04/24 20:09:47, Manish Jethani wrote: > > > > > On 2019/04/24 19:50:38, Sebastian Noack wrote: > > > > > > On 2019/04/24 14:42:23, Manish Jethani wrote: > > > > > > > We need to support calls like `browser.runtime.openOptionsPage()`. > > > > > > > > > > > > > > > > > > I just tested on Firefox (where the promise-based API is supported > > > natively) > > > > > > without the polyfill, and browser.runtime.openOptionsPage(undefined) > > just > > > > > works > > > > > > fine. So we shouldn't emulate an error in that case. > > > > > > > > > > But we're not? Quite the opposite, the additional change I made is about > > > > letting > > > > > `browser.runtime.openOptionsPage()` work. Did I miss something in the > > > rebase? > > > > > > > > Like I read the code here, you throw if the last argument is undefined > given > > > > explicitly, Firefox doesn't. Do I miss something? > > > > > > Are you suggesting we don't throw the error at all? > > > > > > My original idea was to do this so we catch mistakes where we're still using > > > callbacks unknowingly. Of course this can cause trouble when you have > > something > > > like this: > > > > > > function openOptions(callback) > > > { > > > return browser.runtime.openOptionsPage(callback); > > > } > > > > > > openOptions(); > > > > > > It will throw. But that is exactly the intention here. If it throws, we need > > to > > > migrate that code to use promises. After a couple of releases we can remove > > the > > > check for `"undefined"` and finally we remove it for `"function"` as well. > > > > I think a polyfill should behave like the implementation it is imitating. I > see > > your point, but once we drop the polyfill and assuming Chrome by then will > > behave like Firefox, we won't have that feature anymore anyway. > > Fair enough, in that case we have two options: > > 1. Check only for `"function"` > 2. Remove any checks > > I don't think we should have the checks at all if we're sure we've migrated to > promises. Acknowledged.
LGTM 
|
