google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.22k stars 279 forks source link

Upgrade Puppeteer to the latest version as the `page.off()` clean-up function isn't working in the current version #6433

Open hussain-t opened 1 year ago

hussain-t commented 1 year ago

Feature Description

The clean-up function page.off("request", requestHandler) in the useRequestInterception utility function is not working in the current version of the puppeteer. This causes test failures when we invoke the clean-up function, which was returned from the useRequestInterception function. The bug was reported in the Puppeteer repository and was fixed in the latest version.

We should upgrade to the latest version of Puppeteer, in which the above bug was fixed, and there are no breaking changes to our existing tests.

Due to the above issue, we introduced a workaround to intercept the &amp_validate requests in #5460.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

mxbclang commented 1 year ago

@hussain-t Can you please add a priority label to this one? Thank you!

hussain-t commented 1 year ago

Added. Thanks @bethanylang

aaemnnosttv commented 1 year ago

@hussain-t a few things to revise here:

  • The latest puppeteer version should be installed and configured in the project.

The important thing about this issue is that we resolve the problem with page.off – upgrading puppeteer is really more of an implementation detail, but we wouldn't want to consider this done if somehow this behavior was broken again in the latest version :) Similarly, the fix that you've referenced has been merged for several releases now, so there doesn't seem to be a reason why it would need to be the latest version. See https://github.com/puppeteer/puppeteer/commit/d0cb9436a302418086f6763e0e58ae3732a20b62

Secondarily, it's worth noting that we have both puppeteer and puppeteer-core as dependencies (I don't recall why). Another reason to not make the AC specific to puppeteer as we may need to upgrade both.

Would you please revise the AC to be a little more specific as to what we want to get out of this issue? If we already know more/less how to accomplish that, this can be populated in the IB, we don't need to leave that part empty but of course not worth spending too much time there until AC are solidified 👍

ivonac4 commented 7 months ago

@hussain-t Are you still planning to work on this soon? If not, could you please unassign yourself so someone else can pick it up? Thank you!