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

Adds support for using the puppeteer browser cache enabled when request interception is enabled #1885

Conversation

Tyler-Murphy
Copy link

Closes #1809.

This works like puppeteer, in that you must opt in to using the cache when request interception is enabled. The default behavior of disabling the cache is preserved.

@Tyler-Murphy Tyler-Murphy changed the title Adds support for leaving the puppeteer browser cache enabled when request interception is enabled Adds support for using the puppeteer browser cache enabled when request interception is enabled Apr 27, 2021
Copy link
Collaborator

@remusao remusao left a comment

Choose a reason for hiding this comment

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

Very nice, thank you for the detailed documentation and tests. Could you maybe run yarn lint in folder packages/adblocker-puppeteer and maybe yarn format-fix at the root of the repository, I'd like the style of the code base to remain consistent if possible.

@@ -62,7 +62,7 @@ export class BlockingContext {
this.onRequest = (request) => blocker.onRequest(request);
}

public async enable(): Promise<void> {
public async enable({ cacheSafe }: { cacheSafe: boolean }): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's maybe set the default value here instead of enableBlockingInPage since this method is also public.

Suggested change
public async enable({ cacheSafe }: { cacheSafe: boolean }): Promise<void> {
public async enable({ cacheSafe = false }: { cacheSafe: boolean } = {}): Promise<void> {

/**
* The `cacheSafe` can be set to `true` to enable the browser's resource cache. By default, caching is disabled, which is puppeteer's default behavior when request interception is enabled. Learn more here: https://github.com/puppeteer/puppeteer/blob/main/docs/api.md#pagesetrequestinterceptionvalue-cachesafe
*/
public async enableBlockingInPage(page: puppeteer.Page, { cacheSafe = false }: { cacheSafe?: boolean } = {}): Promise<BlockingContext> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's maybe pass whatever we got here to enable() and let this one handle the default value?

Suggested change
public async enableBlockingInPage(page: puppeteer.Page, { cacheSafe = false }: { cacheSafe?: boolean } = {}): Promise<BlockingContext> {
public async enableBlockingInPage(page: puppeteer.Page, opts: { cacheSafe?: boolean }): Promise<BlockingContext> {

let context: undefined | BlockingContext = this.contexts.get(page);
if (context !== undefined) {
return context;
}

context = new BlockingContext(page, this);
this.contexts.set(page, context);
await context.enable();
await context.enable({ cacheSafe });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
await context.enable({ cacheSafe });
await context.enable(opts);

@remusao
Copy link
Collaborator

remusao commented May 4, 2021

After merging this #1903 we now have a few conflicts here, sorry about that. Also a note about tests, we have started using express in this other PR to run a local server during tests, would you mind updated the tests from this PR to use it as well?

@remusao remusao self-requested a review May 8, 2021 11:31
@Tyler-Murphy
Copy link
Author

I think this was made irrelevant by puppeteer/puppeteer#7217, which was released in v10.0.0.

@remusao
Copy link
Collaborator

remusao commented Jun 1, 2021

I think this was made irrelevant by puppeteer/puppeteer#7217, which was released in v10.0.0.

Even better! I will just release a new version of the adblocker with this new v10 tag in the list of Puppeteer versions then.

Tank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: New Feature 🚀 Increment minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can puppeteer browser caching be enabled by allowing request interception and caching at the same time?
2 participants