-
Notifications
You must be signed in to change notification settings - Fork 9.6k
remove support for node before v6 #1519
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
Conversation
| "private": true, | ||
| "engines": { | ||
| "node": ">=0.8.0" | ||
| "node": ">=6" |
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 this match travis's 6.9.1? or vice versa?
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 don't know how much we care about the precision on this. We're going to only be testing 6.9.1, so it makes sense that we shouldn't say we work in case important bugs were fixed before that...but on the other hand we're not testing with the latest v6, so it's also possible that some bug was fixed after 6.9.1 that materially affects execution...
| "private": true, | ||
| "engines": { | ||
| "node": ">=5" | ||
| "node": ">=6" |
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.
same
| }, | ||
| "engines": { | ||
| "node": ">=5" | ||
| "node": ">=6" |
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.
same
|
But do you need 6.9.1 specifically? If not just setting this to 6 will
prevent any confusion since CI will always have the latest 6 version anyway.
…On Jan 24, 2017 23:50, "Brendan Kenny" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lighthouse-extension/package.json
<#1519>:
> @@ -2,11 +2,11 @@
"name": "lighthouse-extension",
"private": true,
"engines": {
- "node": ">=0.8.0"
+ "node": ">=6"
yeah, I don't know how much we care about the precision on this. We're
going to only be testing 6.9.1, so it makes sense that we shouldn't say we
work in case important bugs were fixed before that...but on the other hand
we're not testing with the latest v6, so it's also possible that some bug
was fixed *after* 6.9.1 that materially affects execution...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1519>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAVVtRh_dXIx6OzQ1k0b8ZMPrYFEvIGwks5rVnIYgaJpZM4Lsw2v>
.
|
Similar to how we were specifically testing 4.3.2 before, a key user is stuck with 6.9.1 for at least the immediate future. It's probably best to test that exact version rather than test with latest 6 and keep an ear out for anything that might affect us (e.g. while there's been only a few changes between 6.9.1 and latest 6.9.4, there was one destructuring case that no longer throws, and it's possible we could use it in LH in the future and cause issues for anyone stuck with the slightly older version). |
XhmikosR
left a 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.
Tested everything on my Windows VM, tests are passing.
|
Ready after the resolve |
fixes #449
just basic removal of version checks and logic specifically for getting
--harmonypiped into the CI system. New language features (mostly default parameters, rest args, and[].includes()) can come later.@XhmikosR if I'm missing anything :)