misc: drop support for node 10 #11656

connorjclark · 2020-11-11T21:35:13Z

Drops Node 10. Minimum support is now 12.

I added usage of a Node 12 feature, public instance fields. They are still stage 3, which means eslint's ecmascript parser doesn't support it, so I also changed the eslint parser.

(This is a breaking change–~~do not merge yet)~~

connorjclark · 2020-11-11T21:36:33Z

lighthouse-core/gather/driver.js

@@ -59,6 +59,10 @@ const DEFAULT_PROTOCOL_TIMEOUT = 30000;
 */

 class Driver {
+  online = true;
+  // eslint-disable-next-line no-invalid-this
+  fetcher = new Fetcher(this);


I think this is an error in babel-eslint parser. this is perfectly fine here.

I think this is an error in babel-eslint parser. this is perfectly fine here.

even if it's parsed correctly, I'd imagine eslint isn't going to have any "allow in field initializers" code until they add support for public fields.

(there's also a corner case to handle, where this works here unless the constructor ends up returning something else. hopefully we never land code like that in lighthouse, though :)

what do you mean a constructor can return something else?!

wtf gross https://javascript.info/constructor-new#return-from-constructors

this seems sort of acceptable for functions used as a ctor, but does constructor() really allow this?

wtf gross

haha yes, maybe there's some purity argument somewhere since every function is a potential constructor, but it's a bad argument :)

connorjclark · 2020-11-11T21:36:55Z

package.json

@@ -8,7 +8,7 @@
    "chrome-debug": "./lighthouse-core/scripts/manual-chrome-launcher.js"
  },
  "engines": {
-    "node": ">=10.13"
+    "node": ">=12.13.0"


Should this be 12.0? Or should CI install 12.13.0?

I always get annoyed when packages pick an unnecessarily specific version, anything wrong with 12.x?

12.13 was the first LTS version, so it seems ok to rely on that as a floor. OTOH I don't see much that was added between 12 and 12.13. Promise.allSettled, but anything else?

Promise.allSettled, but anything else

looks like yes, there were a few more things we'll be interested in, like the Intl.NumberFormat improvements (like 'unit') and the V8 7.6 and 7.7 releases. Doesn't seem too onerous to use the October 2019 release as a minimum?

build/build-utils.js

patrickhulce · 2020-11-11T23:15:48Z

is this ready-ready @connorjclark? it feels almost like a WIP given the number of Refresh prompts I had while reviewing 😉

it's OK if this needs a step back to update some of our toolchain first

brendankenny

They are still stage 3, which means eslint's ecmascript parser doesn't support it, so I also changed the eslint parser to babel-eslint.

It was only a few days ago I was saying in chat how much better parser: '@typescript-eslint/parser' is than babel-eslint :P

brendankenny · 2020-11-11T23:20:49Z

lighthouse-core/gather/driver.js

@@ -59,6 +59,10 @@ const DEFAULT_PROTOCOL_TIMEOUT = 30000;
 */

 class Driver {
+  online = true;
+  // eslint-disable-next-line no-invalid-this
+  fetcher = new Fetcher(this);


I think this is an error in babel-eslint parser. this is perfectly fine here.

even if it's parsed correctly, I'd imagine eslint isn't going to have any "allow in field initializers" code until they add support for public fields.

(there's also a corner case to handle, where this works here unless the constructor ends up returning something else. hopefully we never land code like that in lighthouse, though :)

brendankenny · 2020-11-11T23:21:49Z

lighthouse-core/gather/driver.js

+  online = true;
+  // eslint-disable-next-line no-invalid-this
+  fetcher = new Fetcher(this);
+


why not move all the other fields at the same time? Can mark them as @private to sidestep the # question for now :)

Done, but types fail now in the test file...

Thoughts? Drop the private marker?

Thoughts? Drop the private marker?

Ah, yeah, that's unfortunate. Of course in theory we shouldn't be touching private state for tests, but obviously that's often not possible or makes things worse. I guess no @private :/

I marked it.... @pri_vate.

why not move all the other fields at the same time?

too late now, but a good reason IMO would have been that it creates merge conflicts for in progress PRs and we only needed to do 1 small node 12 syntax feature for our v7 goals :)

brendankenny · 2020-11-11T23:23:25Z

package.json

@@ -8,7 +8,7 @@
    "chrome-debug": "./lighthouse-core/scripts/manual-chrome-launcher.js"
  },
  "engines": {
-    "node": ">=10.13"
+    "node": ">=12.13.0"


12.13 was the first LTS version, so it seems ok to rely on that as a floor. OTOH I don't see much that was added between 12 and 12.13. Promise.allSettled, but anything else?

connorjclark · 2020-11-11T23:52:18Z

is this ready-ready @connorjclark? it feels almost like a WIP given the number of Refresh prompts I had while reviewing 😉

it's OK if this needs a step back to update some of our toolchain first

No, I didn't anticipate every tooling to fail (oops). I'll make it clear when it's ready.

This reverts commit f45b270.

patrickhulce

LGTM

lighthouse-core/scripts/dogfood-lhci.sh

patrickhulce · 2020-11-16T21:01:16Z

package.json

    "typescript": "3.9.7"
  },
  "dependencies": {
+    "acorn-node": "^1.7.0",


is this supposed to be a devDep?

ya! I was just about to do it earlier but then VSCode froze and I forgot. Has vscode been freezing for you lately? It's a new thing for me...last week, few times a day I get a 100% cpu usage

hm not really, are you using insiders too?

brendankenny · 2020-11-16T22:51:14Z

package.json

    "@wardpeet/brfs": "2.1.0-0",
+    "acorn-node": "^1.7.0",


shouldn't need this explicitly, it's browserify's dep, just need to delete and have it regenerate in yarn.lock

connorjclark requested a review from a team as a code owner November 11, 2020 21:35

connorjclark requested review from patrickhulce and removed request for a team November 11, 2020 21:35

google-cla bot added the cla: yes label Nov 11, 2020

misc: drop support for node 10

aa0e5c0

connorjclark force-pushed the node_bump branch from 93e2dbf to aa0e5c0 Compare November 11, 2020 21:35

vercel bot deployed to Preview November 11, 2020 21:35 View deployment

devtools-bot assigned patrickhulce Nov 11, 2020

devtools-bot added the waiting4reviewer label Nov 11, 2020

connorjclark commented Nov 11, 2020

View reviewed changes

docs

3792292

vercel bot deployed to Preview November 11, 2020 21:39 View deployment

trying to fix terser..

e4aea1a

vercel bot deployed to Preview November 11, 2020 22:57 View deployment

connorjclark commented Nov 11, 2020

View reviewed changes

build/build-utils.js Outdated Show resolved Hide resolved

hmm

fa2bb28

vercel bot deployed to Preview November 11, 2020 23:03 View deployment

patrickhulce added the breaking label Nov 11, 2020

brendankenny reviewed Nov 11, 2020

View reviewed changes

fix building

a82a852

vercel bot deployed to Preview November 11, 2020 23:27 View deployment

more

e783c4d

connorjclark added the chillin ❄ label Nov 11, 2020

vercel bot deployed to Preview November 11, 2020 23:44 View deployment

use ts parser for eslint

d53d5ae

vercel bot deployed to Preview November 11, 2020 23:54 View deployment

fix ts

b0f3a2c

connorjclark added 2 commits November 12, 2020 12:24

fix stream erroring

7d37a09

Merge remote-tracking branch 'origin/master' into node_bump

2bc4c79

vercel bot deployed to Preview November 12, 2020 20:46 View deployment

connorjclark mentioned this pull request Nov 13, 2020

core(driver): create eval code using interface #10816

Merged

connorjclark added waiting4reviewer and removed waiting4committer labels Nov 13, 2020

Revert "not working"

46e39e7

This reverts commit f45b270.

vercel bot deployed to Preview November 13, 2020 20:51 View deployment

node acorn

7042eb4

vercel bot deployed to Preview November 16, 2020 19:05 View deployment

patrickhulce approved these changes Nov 16, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Nov 16, 2020

pr

f301a59

vercel bot deployed to Preview November 16, 2020 21:14 View deployment

brendankenny reviewed Nov 16, 2020

View reviewed changes

deps

0bb3e24

vercel bot deployed to Preview November 16, 2020 22:53 View deployment

Merge remote-tracking branch 'origin/master' into node_bump

1b78f74

vercel bot deployed to Preview November 16, 2020 22:54 View deployment

paulirish added the 7.0 label Nov 20, 2020

paulirish mentioned this pull request Nov 30, 2020

☂️ 👣 Breaking changes for v7 #11207

Closed

9 tasks

Merge remote-tracking branch 'origin/master' into node_bump

98463ab

vercel bot deployed to Preview December 1, 2020 17:09 View deployment

connorjclark added the land-when-ci-is-green label Dec 1, 2020

Merge remote-tracking branch 'origin/master' into node_bump

d063331

vercel bot deployed to Preview December 1, 2020 23:41 View deployment

devtools-bot merged commit b93b628 into master Dec 2, 2020

devtools-bot deleted the node_bump branch December 2, 2020 00:24

devtools-bot removed the land-when-ci-is-green label Dec 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: drop support for node 10 #11656

misc: drop support for node 10 #11656

connorjclark commented Nov 11, 2020 •

edited

connorjclark Nov 11, 2020

brendankenny Nov 11, 2020

connorjclark Nov 11, 2020

connorjclark Nov 11, 2020 •

edited

brendankenny Nov 12, 2020

connorjclark Nov 11, 2020

patrickhulce Nov 11, 2020

brendankenny Nov 11, 2020

brendankenny Nov 11, 2020

patrickhulce Nov 11, 2020

patrickhulce commented Nov 11, 2020

brendankenny left a comment

brendankenny Nov 11, 2020

brendankenny Nov 11, 2020

connorjclark Nov 11, 2020 •

edited

brendankenny Nov 12, 2020

connorjclark Nov 12, 2020

patrickhulce Nov 12, 2020

brendankenny Nov 11, 2020

connorjclark commented Nov 11, 2020

patrickhulce left a comment

patrickhulce Nov 16, 2020

connorjclark Nov 16, 2020

patrickhulce Nov 16, 2020

brendankenny Nov 16, 2020

misc: drop support for node 10 #11656

misc: drop support for node 10 #11656

Conversation

connorjclark commented Nov 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Nov 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Nov 11, 2020

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Nov 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Nov 11, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Nov 11, 2020 •

edited

connorjclark Nov 11, 2020 •

edited

connorjclark Nov 11, 2020 •

edited