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

feat(requestinterception): remove cacheSafe flag #7217

Merged
merged 2 commits into from
May 20, 2021
Merged

feat(requestinterception): remove cacheSafe flag #7217

merged 2 commits into from
May 20, 2021

Conversation

Androbin
Copy link
Contributor

@Androbin Androbin commented May 7, 2021

Reverts #6996

#1154 @aslushnikov disabled the cache by default.

There were two reasons for this change:

@google-cla google-cla bot added the cla: yes label May 7, 2021
@Androbin
Copy link
Contributor Author

Androbin commented May 7, 2021

@OrKoN @jschfflr PTAL

This was referenced May 7, 2021
Closed
Closed
@jschfflr jschfflr self-requested a review May 10, 2021 11:47
@jschfflr
Copy link
Contributor

Could you elaborate why you chose to remove this feature again?

@Androbin
Copy link
Contributor Author

There was a bug that forced us to implicitly disable the cache when enabling interception.
Some people were not affected by this bug and wanted to keep the cache enabled.
This flag was added to allow people to override that behavior while keeping it the default behavior.
Now the bug in question is fixed and there is no point in implicitly disabling the cache anymore.
This PR removes the logic around disabling the cache implicitly, including the workaround flag.
It is still possible to disable the cache explicitly using setCacheDisabled.

@jschfflr
Copy link
Contributor

Awesome, thanks for the explanation!

jschfflr
jschfflr approved these changes May 12, 2021
View reviewed changes
Copy link
Contributor

@jschfflr jschfflr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jschfflr
Copy link
Contributor

Just to make sure I understand this correctly: This cl replaces #7215, right?

@Androbin
Copy link
Contributor Author

Just to make sure I understand this correctly: This cl replaces #7215, right?

Yes, that is correct.

@jschfflr
Copy link
Contributor

Can you update this change? I think the Node.js v10 support was recently dropped and it seems like the check is still waiting for it...

@jschfflr jschfflr merged commit d01aa6c into puppeteer:main May 20, 2021
@Androbin Androbin deleted the requestinterception-2 branch May 20, 2021 13:16
@Tyler-Murphy
Copy link

api.md says:

NOTE Enabling request interception disables page caching.

Does that need to be removed? I'm not sure, because I might have misunderstood the effect of this change.

@Tyler-Murphy Tyler-Murphy mentioned this pull request May 31, 2021
Closed
@Androbin Androbin mentioned this pull request Jun 1, 2021
Merged
@Androbin
Copy link
Contributor Author

Androbin commented Jun 1, 2021

@Tyler-Murphy You are right, thanks for spotting this, I have created #7304 to update the docs.

@jschfflr
Copy link
Contributor

jschfflr commented Jun 1, 2021

@Tyler-Murphy Thanks for reaching out!

This was referenced May 30, 2022
Closed
Closed
This was referenced May 30, 2022
Closed
Closed
Closed
This was referenced May 30, 2022
Closed
Closed
@Androbin Androbin mentioned this pull request Dec 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jschfflr jschfflr jschfflr approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Androbin @jschfflr @Tyler-Murphy