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

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

@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!

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.

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants