-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
deps: move to minimum Node 14 #13243
Conversation
oh yeah, new stuff :) Main new features available are nullish coalescing, optional chaining, and private class fields/methods. Node APIs also continue to improve, like the As in the past, no need to go out of your way to update old code to use this, we'll just ease into it, updating organically as other code changes land. It's also possible we won't be able to use much of the new JS stuff until we've transitioned off of browserify (see #12134). |
@@ -52,7 +52,7 @@ The Chrome extension was available prior to Lighthouse being available in Chrome | |||
|
|||
The Node CLI provides the most flexibility in how Lighthouse runs can be configured and reported. Users who want more advanced usage, or want to run Lighthouse in an automated fashion should use the Node CLI. | |||
|
|||
_Lighthouse requires Node 12 LTS (12.20) or later._ | |||
_Lighthouse requires Node 14 LTS (14.x) or later._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are now entering the phase where the master LH readme is misleading for the current release :)
Should we add a giant link at the top of the readme to the last 8.x readme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question, though I feel like in this case it doesn't reallllly matter. Node 14 became LTS a year ago, I kind of feel like people should be updated anyways :) At worst they'll feel forced to update and LH 8 will still work with it
@@ -9,7 +9,7 @@ | |||
"smokehouse": "./lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js" | |||
}, | |||
"engines": { | |||
"node": ">=12.20.0 12 || >=14.13 14 || >=15" | |||
"node": ">=14.15" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't have a clear feeling for what minimum minor version we should require here. 14.15 is the first LTS 14 release, so it makes sense as a baseline, but there's also been critical security updates since then, so we should definitely not be running that version for testing (and shouldn't be encouraging anyone else to do so either)
Is the intention of Node LTS that since it has long term support with no breaking changes, it's reasonable to always require the latest 14.x?
It's kind of academic right now because unlike Node 12 there haven't (yet) been important feature backports to Node 14 in versions after the first LTS release (like we ended up needing with ESM and Node >= 12.20.0), but it would still be nice to have a coherent vision on if we should consider updating the required Minor version as a breaking change for Lighthouse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can assume the cost of any org updating from 14.x to a higher 14.x is sufficiently low, that we should go higher if we have reason to.
But also, it's hard to know if we have reason to ahead of time, without closely reading all the changelogs since then and thinking real hard about if anything will tie our hands wrt. upcoming refactors we want to do.
so... I'm for putting the latest 14.x release as our minimum.
@@ -4,6 +4,8 @@ | |||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | |||
*/ | |||
|
|||
import {EventEmitter} from 'events'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is from the updated Node types. No longer a global NodeJS.EventEmitter
type, it's an interface, so need to extend the actual class declaration.
isDevtools?: boolean; | ||
isLightrider?: boolean; | ||
} | ||
declare global { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also from the update node types: global
is now just type globalThis
, so to add to global
need to augment the global scope
part of #12614
With Node 17 out and Node 16 LTS planned for next week, this moves us to requiring Node 14 (now the Maintenance LTS) as a minimum requirement.
The changes here are
full-icu
by default (it will still log instructions if it's run in a Node purposefully built assmall-icu
and a non-en-US
locale is requested)rmdirSync
->rmSync
@types/node