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

Update to puppeteer 3.0+ #949

Closed
samreid opened this issue May 26, 2020 · 14 comments
Closed

Update to puppeteer 3.0+ #949

samreid opened this issue May 26, 2020 · 14 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented May 26, 2020

From #947 we would like to update puppeteer to a more recent version:

https://github.com/puppeteer/puppeteer says

Note: Prior to v1.18.1, Puppeteer required at least Node v6.4.0. Versions from v1.18.1 to v2.1.0 rely on Node 8.9.0+. Starting from v3.0.0 Puppeteer starts to rely on Node 10.18.1+. All examples below use async/await which is only supported in Node v7.6.0 or greater.

But some PhET developers were using Node <10 and @zepumph determined that our build server is using Node v10.16.3. Hence this would need to upgrade before we can move forward here.

We don't have an immediate need to update puppeteer for any particular bugfix or feature, but for stability and maintainability we should start to move this forward. Tagging for developer meeting about moving this forward.

@samreid samreid changed the title Update to puppeteer 3.0 Update to puppeteer 3.0+ May 26, 2020
@mattpen
Copy link
Contributor

mattpen commented May 28, 2020

But some PhET developers were using Node <10

There aren't any versions of node < 10 that are currently supported, see https://nodejs.org/en/about/releases/. Is there some reason devs need to be on older versions?

I'm also planning to move the phet servers to Node 14 in October, but I don't think that should affect this conversation.

@mattpen
Copy link
Contributor

mattpen commented May 28, 2020

We'll defer the upgrade to puppeteer 3.0+ until October when we upgrade node to 14.

@ariel-phet
Copy link
Contributor

Calendar reminder made

@samreid
Copy link
Member Author

samreid commented Nov 15, 2020

I updated to puppeteer ^5.4.1 in my working copy, and tested the following parts:

  • precommit hook for unit tests
  • generate phet-io-api
  • aqua/fuzzOneSim
  • cheerpj get preloads
  • unitTestBatch
  • pdom comparison
  • puppeteerHelpCT
  • installer builder proxy

@samreid
Copy link
Member Author

samreid commented Nov 15, 2020

#947 (comment) says:

@jessegreenberg and @zepumph reported problems with puppeteer when running older versions of Node. When @jessegreenberg updated to a recent version of node the problem went away. In @zepumph case, he was running node 10 and npm install was itself failing. We decided for now to downgrade the puppeteer version and discuss as a team how we can update this.

Here is a patch that upgrades puppeteer to 5.4.1. After applying this patch, you can npm prune and npm update in chipper, aqua and perennial.

@zepumph when you have time, can you confirm that this new version works OK on your side now that we are using Node 14?

Index: perennial/package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- perennial/package.json	(revision 819d7878cbf333f929c1cd4af975ca5a91b4f29f)
+++ perennial/package.json	(date 1605449622186)
@@ -26,7 +26,7 @@
     "winston": "^0.9.0",
     "winston-loggly": "^1.3.1",
     "xml2js": "^0.4.15",
-    "puppeteer": "~2.1.1",
+    "puppeteer": "^5.4.1",
     "html-differ": "1.4.0"
   },
   "eslintConfig": {
Index: aqua/package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- aqua/package.json	(revision d25054801465932bd48f4fb3060ae05e7dcbceaf)
+++ aqua/package.json	(date 1605449557546)
@@ -11,7 +11,7 @@
     "lodash": "^4.17.10",
     "ncp": "^2.0.0",
     "rimraf": "^2.5.4",
-    "puppeteer": "~2.1.1",
+    "puppeteer": "^5.4.1",
     "winston": "^0.9.0"
   },
   "eslintConfig": {
Index: chipper/package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- chipper/package.json	(revision 5131b25ea4349d9472bae89ad2c19f04ac7ab412)
+++ chipper/package.json	(date 1605449710689)
@@ -22,7 +22,7 @@
     "md5": "~2",
     "node-html-encoder": "~0.0.2",
     "pngjs": "~0.4.0",
-    "puppeteer": "~2.1.1",
+    "puppeteer": "^5.4.1",
     "qunit": "~2.10.0",
     "request": "^2.87.0",
     "request-promise-native": "^1.0.7",
Index: chipper/js/grunt/generateTestHTML.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- chipper/js/grunt/generateTestHTML.js	(revision 5131b25ea4349d9472bae89ad2c19f04ac7ab412)
+++ chipper/js/grunt/generateTestHTML.js	(date 1605461917122)
@@ -32,7 +32,7 @@
     bodystart: '<div id="qunit"></div>\n<div id="qunit-fixture"></div>' + ( repo === 'scenery' ? '<div id="display"></div>' : '' ),
 
     // Add QUnit JS
-    addedPreloads: [ '../sherpa/lib/qunit-2.10.0.js', '../chipper/js/sim-tests/qunit-connector.js' ],
+    addedPreloads: [ '../sherpa/lib/qunit-2.12.0.js', '../chipper/js/sim-tests/qunit-connector.js' ],
 
     // Do not show the splash screen
     stripPreloads: [ '../joist/js/splash.js' ],

@samreid samreid assigned zepumph and unassigned samreid Nov 15, 2020
@zepumph
Copy link
Member

zepumph commented Nov 16, 2020

I applied the patch and was able to successfully build GAO for phet-io brand, and I ran hook-pre-commit.js from axon via the command line. Everything worked great! I also confirmed by adding an erroring test in axon, and the hoo-pre-commit.js failed.

@zepumph
Copy link
Member

zepumph commented Nov 16, 2020

Was there anything specific that I was supposed to test?

@zepumph zepumph assigned samreid and unassigned zepumph Nov 16, 2020
@samreid
Copy link
Member Author

samreid commented Nov 16, 2020

That's perfect, thanks. I'll move forward for next steps.

samreid added a commit to phetsims/perennial that referenced this issue Nov 16, 2020
samreid added a commit that referenced this issue Nov 16, 2020
samreid added a commit to phetsims/aqua that referenced this issue Nov 16, 2020
@samreid
Copy link
Member Author

samreid commented Nov 16, 2020

I updated to puppeteer ^5.4.1 in the commits. Please npm prune and npm update in aqua, chipper and perennial at your convenience and report any issues in #949

@samreid
Copy link
Member Author

samreid commented Nov 16, 2020

I'd like to hear that:

  • pdom comparison
  • puppeteerHelpCT
  • installer builder proxy

Are working satisfactorily before closing this issue. Marking to touch base at developer meeting.

@samreid samreid removed their assignment Nov 16, 2020
@Denz1994 Denz1994 self-assigned this Nov 23, 2020
@Denz1994
Copy link
Contributor

From dev meeting on 11/23/20:

MK: We can check off pdom comparison.

DB: I'll investigate the usage in the installer builder.

MK: I'll test puppeteerHelpCT.

SR: After updating everything via Webstorm, building seemed okay as a first pass. Maybe we are too conservative with not updating.

@Denz1994
Copy link
Contributor

@samreid Updating puppeteer wouldn't impair the installer builder usage. Reassigning to @samreid.

Just for some context, InstallerBuilderProxy was used as a prototype for server-side rendering of dynamic content on HTML pages. It isn't core to the installer builder's build process.

@Denz1994 Denz1994 assigned samreid and unassigned Denz1994 Nov 30, 2020
@samreid samreid assigned zepumph and unassigned samreid Nov 30, 2020
@zepumph
Copy link
Member

zepumph commented Dec 3, 2020

Anything else here @samreid? I checked my boxes after verifying.

@zepumph zepumph assigned samreid and unassigned zepumph Dec 3, 2020
@samreid
Copy link
Member Author

samreid commented Dec 5, 2020

Thanks, it seems we are good to go, closing.

@samreid samreid closed this as completed Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants