deps(puppeteer): upgrade to 9.1.1

[connorjclark]

ref #12145 (comment)

Wow, that's a lot of major version bumps.

I also removed the @types module, since puppeteer now has its own types.

[connorjclark]

i only upgraded in the hopes that the type error from session.js would be resolved, but it wasn't. might as well keep it tho?

[adamraine]

Yeah, may as well. Similar change was made in #12199 but I don't know when we will be able to merge that (once timing issues are resolved).

[brendankenny]

If pptr is going to be the new way of doing things, can we align with their devtools-protocol? That should eliminate this error.

[connorjclark]

That would put a limit on how soon we can adopt new protocol types: only as soon as Puppeteer does a release.

[brendankenny]

I agree we can't let puppeteer types dictate when we update our version of the protocol, but maybe we can come up with a policy in the near future since we're hitching our wagon to puppeteer and we'll have to figure out an update cadence for that (and/or puppeteer-core) anyways (it makes sense to use its types since we'll use puppeteer as our protocol manager, but we're also deviating from puppeteer's suggested Chrome version policy, so the protocol types don't necessarily need to be what puppeteer ships with).

For just this change, though:

• we should avoid suppressing an error if there's not much downside to avoiding the error
• the protocol definitions are huge, and typescript has to compile and check both versions. Looking at the tsc diagnostics, it's an extra 18k types and (in totally unscientific testing) an extra second of type check time just for the doubled protocol file and comparison in session.js.
• we're updating the protocol definitions for non-pressing reasons after six months of no issues, so one-month old definitions seem fine to use for now (they still include everything core(fetcher): fetch over protocol #12199 will need)

[connorjclark]

I agree we can't let puppeteer types dictate when we update our version of the protocol ...

we should avoid suppressing an error if there's not much downside to avoiding the error

I don't understand how to take these two statements. If we drop devtools-protocol for puppeteer types, we are immediately tying ourselves to puppeteer. As soon as a change in the protocol comes in that we need (ex: the next Issue type), we are stuck unless Puppeteer updates in-sync.

If we want to move towards this we should ask Puppeteer if there's a way to make puppeteer use devtools-protocol directly, then we could override the transitive dep in our package.json.

[brendankenny]

I agree we can't let puppeteer types dictate when we update our version of the protocol ...

we should avoid suppressing an error if there's not much downside to avoiding the error

I don't understand how to take these two statements.

can take them like:

I agree we can't let puppeteer types dictate when we update our version of the protocol, but maybe we can come up with a policy in the near future...
For just this change, though: we should avoid suppressing an error if there's not much downside to avoiding the error

:D

I'm not suggesting we give up protocol decisions to puppeteer as a policy going forward, I'm suggesting we use [email protected] for this update in particular since that's what puppeteer 8 is using, there's no downsides to using that version, and it avoids the _session.send() error for now.

[connorjclark]

request interception seems to be broken for XHR (fetch) ...

bisected to v2.0.0 https://github.com/puppeteer/puppeteer/releases/tag/v2.0.0 . I don't think it is related to puppeteer code, it must be a regression in Chrome because using 1.19.0 but with latest Chrome/canary results in the same error :(

broke somewhere here: 78.0.3882.0 (r686378) -> 79.0.3942.0 (r706915)

[adamraine]

Yeah, may as well. Similar change was made in #12199 but I don't know when we will be able to merge that (once timing issues are resolved).

[connorjclark]

broke somewhere here: 78.0.3882.0 (r686378) -> 79.0.3942.0 (r706915)

I bisected with this script (and using puppeteer v8):

#!/bin/bash

# python bisect-builds.py --verify-range -a mac -g 686378 -b 706915 --use-local-cache --not-interactive -c "bash scripts/puppeteer-upgrade/script.sh %p"

LH=~/code/lighthouse

export PUPPETEER_EXECUTABLE_PATH=$1

cd $LH
if yarn jest lighthouse-viewer/test/viewer-test-pptr.js -t 'should call out to PSI with all categories by default'; then
  echo "ok $1"
  exit 0
else
  echo "bad $1"
  exit 1
fi

But it immediately failed because 686378 was not a good revision.

[connorjclark]

Not sure if I missed this before, or if the newest version fixed something, but here's the error message I see now:

Access to fetch at 'https://www.googleapis.com/pagespeedonline/v5/runPagespeed?url=https%3A%2F%2Fwww.example.com&strategy=mobile&locale=es&utm_source=utm&category=seo&category=pwa&key=AIzaSyAjcDRNN9CX9dCazhqI4lGR7yyQbkd_oYE' from origin 'http://localhost:10200' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

Setting a header fixed the tests.

Searched briefly for why this change in behavior happened, but couldn't find anything.

[connorjclark]

pssssst @brendankenny :)