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.

Sign up for GitHub

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

Jump to bottom

chore(adblock): setup priority #321

Merged
merged 5 commits into from
Oct 19, 2021
Merged

chore(adblock): setup priority #321

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fbbffa0
chore(adblock): setup priority
Kikobeats Oct 19, 2021
4524583
Update index.js
Kikobeats Oct 19, 2021
8a8ee97
chore(adblock): setup priority out of async block
Kikobeats Oct 19, 2021
3f05c3d
chore: add continue debug log
Kikobeats Oct 19, 2021
e51ca03
chore: register aborTypes before adblock
Kikobeats Oct 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 14 additions & 10 deletions packages/goto/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const engine = PuppeteerBlocker.deserialize(

engine.on('request-blocked', ({ url }) => debugAdblock('block', url))
engine.on('request-redirected', ({ url }) => debugAdblock('redirect', url))
engine.setRequestInterceptionPriority(1)

const isEmpty = val => val == null || !(Object.keys(val) || val).length

Expand Down Expand Up @@ -158,6 +159,19 @@ module.exports = ({

const prePromises = []

if (abortTypes.length > 0) {
await page.setRequestInterception(true)
page.on('request', req => {
const resourceType = req.resourceType()
if (!abortTypes.includes(resourceType)) {
debug('continue', { url: req.url(), resourceType })
return req.continue(req.continueRequestOverrides(), 2)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I am meddling where I don't belong 😅 This .continue({}, 2) will cause the request to continue even if adBlocker calls .abort('..',1). I think your intent might be to defer to other handlers by using .continue({}, 0) if abortTypes has no opinion about it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that was the intention; should I call isInterceptResolutionHandled to be sure about that?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When #7796 is approved (I hope soon), that method will be available and should definitely be used.

Also see the PR for a discussion on when to use a custom priority to continue a request.

Again sorry to jump in, I just found this issue and thought it was interesting because I’m learning a lot by how people are implementing support for cooperative mode.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem 🙂 I did my best implementing the API and since there is not too examples out there probably the code can be rewritten in a better way

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you decide to change anything, I suggest using .continue({...},0) for this case :)

}
debug('abort', { url: req.url(), resourceType })
return req.abort('blockedbyclient', 2)
})
}

if (adblock) {
prePromises.push(
run({
Expand Down Expand Up @@ -271,16 +285,6 @@ module.exports = ({

await Promise.all(prePromises.concat(applyEvasions))

if (abortTypes.length > 0) {
await page.setRequestInterception(true)
page.on('request', req => {
const resourceType = req.resourceType()
if (!abortTypes.includes(resourceType)) return req.continue()
debug('abort', { url: req.url(), resourceType })
return req.abort('blockedbyclient')
})
}

const { value } = await run({
fn: html ? page.setContent(html, args) : page.goto(url, args),
timeout,
Expand Down