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

deps(puppeteer): upgrade to 9.1.1 #12284

Merged
merged 7 commits into from
May 21, 2021
Merged

deps(puppeteer): upgrade to 9.1.1 #12284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0b37dc6
deps(puppeteer): upgrade to 8.0.0
connorjclark Mar 23, 2021
d6d5c4c
update
connorjclark Mar 23, 2021
e5bf022
timeout
connorjclark Apr 12, 2021
669796a
Merge remote-tracking branch 'origin/master' into up-pup
connorjclark Apr 12, 2021
42324ea
Merge remote-tracking branch 'origin/master' into up-pup
connorjclark May 20, 2021
1969a3c
yay
connorjclark May 21, 2021
fefebf1
tsc
connorjclark May 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clients/test/extension/popup-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
/* eslint-env jest */

const path = require('path');
const puppeteer = require('../../../node_modules/puppeteer/index.js');
const puppeteer = require('puppeteer');
const {DEFAULT_CATEGORIES, STORAGE_KEYS} =
require('../../extension/scripts/settings-controller.js');

Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/fraggle-rock/gather/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,4 +107,3 @@ class ProtocolSession {
}

module.exports = ProtocolSession;

6 changes: 2 additions & 4 deletions lighthouse-core/scripts/pptr-run-devtools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ const fs = require('fs');
const readline = require('readline');
const yargs = require('yargs/yargs');

/** @typedef {{result?: {value?: string, objectId?: number}, exceptionDetails?: object}} RuntimeEvaluateResponse */

const argv = yargs(process.argv.slice(2))
.usage('$0 [url]')
.help('help').alias('help', 'h')
Expand Down Expand Up @@ -116,7 +114,7 @@ async function testPage(browser, url) {
.catch(reject);
});

/** @type {RuntimeEvaluateResponse|undefined} */
/** @type {Omit<puppeteer.Protocol.Runtime.EvaluateResponse, 'result'>|undefined} */
let startLHResponse;
while (!startLHResponse || startLHResponse.exceptionDetails) {
startLHResponse = await session.send('Runtime.evaluate', {
Expand All @@ -125,7 +123,7 @@ async function testPage(browser, url) {
}).catch(err => ({exceptionDetails: err}));
}

/** @type {RuntimeEvaluateResponse} */
/** @type {puppeteer.Protocol.Runtime.EvaluateResponse} */
const remoteLhrResponse = await session.send('Runtime.evaluate', {
expression: sniffLhr,
awaitPromise: true,
Expand Down
8 changes: 4 additions & 4 deletions lighthouse-core/test/fraggle-rock/gather/session-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ describe('ProtocolSession', () => {

puppeteerSession.on('Foo', regularListener);
puppeteerSession.on('*', allListener);
puppeteerSession.emit('Foo', 1, 2, 3);
puppeteerSession.emit('Bar', 1, 2, 3);
puppeteerSession.emit('Foo', 1);
puppeteerSession.emit('Bar', 1);

expect(regularListener).toHaveBeenCalledTimes(1);
expect(allListener).toHaveBeenCalledTimes(2);
Expand All @@ -53,8 +53,8 @@ describe('ProtocolSession', () => {

puppeteerSession.on('Foo', regularListener);
puppeteerSession.on('*', allListener);
puppeteerSession.emit('Foo', 1, 2, 3);
puppeteerSession.emit('Bar', 1, 2, 3);
puppeteerSession.emit('Foo', 1);
puppeteerSession.emit('Bar', 1);

expect(regularListener).toHaveBeenCalledTimes(1);
expect(allListener).toHaveBeenCalledTimes(2);
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-treemap/test/treemap-test-pptr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

/* global document, window */

const puppeteer = require('../../node_modules/puppeteer/index.js');
const puppeteer = require('puppeteer');
const {server} = require('../../lighthouse-cli/test/fixtures/static-server.js');
const portNumber = 10200;
const treemapUrl = `http://localhost:${portNumber}/dist/gh-pages/treemap/index.html`;
Expand Down
25 changes: 19 additions & 6 deletions lighthouse-viewer/test/viewer-test-pptr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
const path = require('path');
const fs = require('fs');
const assert = require('assert').strict;
const puppeteer = require('../../node_modules/puppeteer/index.js');
const puppeteer = require('puppeteer');

const {server} = require('../../lighthouse-cli/test/fixtures/static-server.js');
const portNumber = 10200;
Expand Down Expand Up @@ -148,25 +148,38 @@ describe('Lighthouse Viewer', () => {
});

describe('PSI', () => {
/** @type {Partial<puppeteer.ResponseForRequest>} */
let interceptedRequest;
/** @type {Partial<puppeteer.ResponseForRequest>} */
let psiResponse;

const sampleLhrJson = JSON.parse(fs.readFileSync(sampleLhr, 'utf-8'));
/** @type {Partial<puppeteer.ResponseForRequest>} */
const goodPsiResponse = {
status: 200,
contentType: 'application/json',
body: JSON.stringify({lighthouseResult: sampleLhrJson}),
headers: {
'Access-Control-Allow-Origin': '*',
},
};
/** @type {Partial<puppeteer.ResponseForRequest>} */
const badPsiResponse = {
status: 500,
contentType: 'application/json',
body: JSON.stringify({error: {message: 'Test error'}}),
headers: {
'Access-Control-Allow-Origin': '*',
},
};

// Sniffs just the request made to the PSI API. All other requests
// fall through.
// To set the mocked PSI response, assign `psiResponse`.
// To read the intercepted request, use `interceptedRequest`.
/**
* Sniffs just the request made to the PSI API. All other requests
* fall through.
* To set the mocked PSI response, assign `psiResponse`.
* To read the intercepted request, use `interceptedRequest`.
* @param {import('puppeteer').HTTPRequest} request
*/
function onRequest(request) {
if (request.url().includes('https://www.googleapis.com')) {
interceptedRequest = request;
Expand Down Expand Up @@ -198,7 +211,7 @@ describe('Lighthouse Viewer', () => {
await viewerPage.goto(url);

// Wait for report to render.
await viewerPage.waitForSelector('.lh-metrics-container');
await viewerPage.waitForSelector('.lh-metrics-container', {timeout: 5000});

const interceptedUrl = new URL(interceptedRequest.url());
expect(interceptedUrl.origin + interceptedUrl.pathname)
Expand Down
5 changes: 2 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@
"@types/lodash.isequal": "^4.5.2",
"@types/lodash.set": "^4.3.6",
"@types/node": "*",
"@types/puppeteer": "1.19.x",
"@types/raven": "^2.5.1",
"@types/resize-observer-browser": "^0.1.1",
"@types/semver": "^5.5.0",
Expand All @@ -130,7 +129,7 @@
"cpy": "^7.0.1",
"cross-env": "^7.0.2",
"csv-validator": "^0.0.3",
"devtools-protocol": "0.0.859327",
"devtools-protocol": "0.0.863986",
Copy link
Collaborator Author

@connorjclark connorjclark Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

"eslint": "^7.23.0",
"eslint-config-google": "^0.9.1",
"eslint-plugin-local-rules": "0.1.0",
Expand All @@ -151,7 +150,7 @@
"package-json-versionify": "^1.0.4",
"prettier": "^1.14.3",
"pretty-json-stringify": "^0.0.2",
"puppeteer": "^1.19.0",
"puppeteer": "^9.1.1",
"tabulator-tables": "^4.9.3",
"terser": "^5.3.8",
"typed-query-selector": "^2.4.0",
Expand Down
Loading