Opened 9 months ago
Closed 9 months ago
#7406 closed change (fixed)
Stop using Utils.runAsync
Reported by: | kzar | Assignee: | mjethani |
---|---|---|---|
Priority: | P2 | Milestone: | |
Module: | Core | Keywords: | manifestv3 |
Cc: | mjethani, sebastian, hfiguiere, greiner | Blocked By: | #7381 |
Blocking: | #7338 | Platform: | Unknown / Cross platform |
Ready: | yes | Confidential: | no |
Tester: | Unknown | Verified working: | no |
Review URL(s): |
https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/40 |
Description (last modified by mjethani)
Background
With #7338 we're no longer able to use some browser APIs from our background page. One thing we can no longer provide therefore is the Utils.runAsync function.
What to change
Stop using Utils.runAsync, namely in adblockpluscore/lib/downloader.js
Integration notes
Remove Utils.runAsync() from lib/utils.js in adblockpluschrome.
Hints for testers
Same as #7376.
Change History (13)
comment:1 Changed 9 months ago by mjethani
- Cc hfiguiere added
comment:2 Changed 9 months ago by mjethani
- Component changed from Unknown to Core
comment:3 Changed 9 months ago by kzar
- Keywords manifestv3 added
comment:4 Changed 9 months ago by sebastian
- Description modified (diff)
comment:5 follow-up: ↓ 8 Changed 9 months ago by mjethani
comment:6 Changed 9 months ago by mjethani
- Cc greiner added
comment:7 Changed 9 months ago by mjethani
- Blocked By 7381 added
comment:8 in reply to: ↑ 5 Changed 9 months ago by mjethani
Replying to mjethani:
- In the download() method in lib/downloader.js in adblockpluscore, we return a promise that resolves or rejects when the download is complete or has failed; this would make it asynchronous anyway
Just to be clear, it doesn't have to return the promise for the purpose of resolving this issue. It could just keep the promise internal. In fact it could just use setTimeout() with a timeout of 0 as Utils.runAsync() does.
I have to point this out because returning a promise would entail further changes in the _download() method's implementation to make sure everything resolves/rejects correctly.
comment:9 Changed 9 months ago by sebastian
For reference, the logic in Utils.runAsync() was a workaround that makes sure to call the callback not before all background scripts have been loaded/executed. There is a good chance that this workaround is no longer necessary, as most of our code is bundled together with webpack, and it might have been a browser bug in the first place that deferred loading of background scripts.
comment:10 Changed 9 months ago by mjethani
- Owner set to mjethani
comment:11 Changed 9 months ago by mjethani
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:12 Changed 9 months ago by abpbot
A commit referencing this issue has landed:
Issue 7406 - Replace Utils.runAsync() with promise
comment:13 Changed 9 months ago by mjethani
- Description modified (diff)
- Resolution set to fixed
- Status changed from reviewing to closed
I have looked into this and I would like to suggest the following: