-
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
misc: drop support for Node before 10.13 (LTS) #8117
Conversation
@@ -139,9 +139,8 @@ async function prepareAssets(artifacts, audits) { | |||
} | |||
|
|||
/** | |||
* Generates a JSON representation of traceData line-by-line to avoid OOM due to very large traces. | |||
* COMPAT: As of Node 9, JSON.parse/stringify can handle 256MB+ strings. Once we drop support for | |||
* Node 8, we can 'revert' PR #2593. See https://stackoverflow.com/a/47781288/89484 |
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.
even though we can do this now, I think we like the one trace event per line format, so it made sense to drop the old comment.
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.
yes, we love it
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.
LGTM
.appveyor.yml
Outdated
platform: x86 | ||
# other Node versions are skipped, as appveyor only allows 1 concurrent job | ||
# and we want appveyor finishing ASAP. see #2382 | ||
# - nodejs_version: "9" | ||
# - nodejs_version: "11" |
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.
should we even bother to keep this in here?
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.
should we even bother to keep this in here?
lol, no :)
Hang on though, tsc isn't happy with new node types |
there was a conflict with the new node types assuming that there would always be a The last holdout fortunately fixes a long-time TODO (that we probably would have missed again for the major version change :), but on the downside is more annoying to review :) |
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.
still LGTM!
@@ -202,10 +196,6 @@ declare global { | |||
group: string; | |||
} | |||
|
|||
export interface LighthouseError extends Error { |
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.
might have been nice to just export our class as this type so we could easily reference it as LH.LighthouseError
still but I don't feel strongly
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.
yeah, I'd like to add this to our public types (when those actually exist :)
fixes #7215 and part of #7752