Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

drop support for Nodes 0.10, 0.12, and 5 #14503

Merged
merged 2 commits into from
Nov 3, 2016

Conversation

othiym23
Copy link
Contributor

@othiym23 othiym23 commented Nov 2, 2016

We still 💖 u, Node.js 0.10, but we gotta keep up with the times. This is not a license to go wild with ES6isms. npm still works (most of the time) back to 0.8, and that's in large part because of the project's conservatism about sticking to plain ES5 in both its own code base and its npm-controlled dependencies. Dependencies not under the team's control are already hard-deprecating 0.10, though, and the build matrix already leads to long Travis times.

This patch also adds Node 7 to the CI matrix, but build should overall be significantly faster because there are, overall, two fewer versions to test.

@zkat
Copy link
Contributor

zkat commented Nov 2, 2016

🔥 🤕 🔥 🎉 !!! 🐑 👯

@legodude17
Copy link
Contributor

Why are the ci checks still running? I think this is a good idea, I hope that almost no one uses those versions anymore.

@othiym23
Copy link
Contributor Author

othiym23 commented Nov 3, 2016

Around 10% of the traffic to the primary npm registry comes from users running Node 0.10, in part because the newer LTS versions have been slow to filter out to the distribution package managers that enterprise ops teams treat as their trusted builds. That's enough that we're not going to move to break anybody, but small enough that we can turn this particular crank.

Why are the ci checks still running?

Because we haven't landed this PR yet?

@legodude17
Copy link
Contributor

Because we haven't landed this PR yet?

Still, I think that 45 minutes is a little long.

@othiym23
Copy link
Contributor Author

othiym23 commented Nov 3, 2016

npm's repo is public, so we're on Travis CI's free plan, so we have to wait when the number of pending builds is high. Also, it's ~15 minutes per version to build, and ~1 hour for the build that's doing coverage checking.

That said, I see that I need to update the (reassuringly complete) tests to account for the versions we've dropped support for.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.292% when pulling 257bb21 on othiym23/rip-off-the-bandage into 30d75e7 on release-next.

@othiym23
Copy link
Contributor Author

othiym23 commented Nov 3, 2016

@phated Do you have any thoughts on this? As we've discussed previously, this is just dropping versions from the test matrix, not a statement of intent to migrate the code base to ES6 and newer Node APIs (although having fewer streams edge cases to worry about is definitely something I look forward to).

I'd have to discuss it more with the rest of the CLI team, but in practice I think this means that we're only going to gate merging of PRs and updating of dependencies on whether the build stays green, but would be amenable to merging community patches that preserve / restore compatibility with older Node releases.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.292% when pulling b68bfd27fefbbab8fd66ee81a2fc56cd68eb7c19 on othiym23/rip-off-the-bandage into 30d75e7 on release-next.

@phated
Copy link
Contributor

phated commented Nov 3, 2016

@othiym23 thanks for copying me.

Would you be willing to add those versions as allow_failures in the travis matrix? That way community members can see 0.10/0.12 failures directly on travis without having to pull down and run tests locally all the time. You can also enable fast_finish which will mark the build a success before those allow_failures are finished running. I think this handles most of the concerns about speed, etc.

It would be nice to see an adjustment to the linting rules to fail on >ES5 language constructs but I think that would be hard due to standard which has es6: true and the ecmaVersion: 8 parser. Seeing as you gate all the PRs, I'm confident ES6isms won't likely enter the code base.

Copy link
Contributor

@iarna iarna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@othiym23
Copy link
Contributor Author

othiym23 commented Nov 3, 2016

Would you be willing to add those versions as allow_failures in the travis matrix? That way community members can see 0.10/0.12 failures directly on travis without having to pull down and run tests locally all the time. You can also enable fast_finish which will mark the build a success before those allow_failures are finished running. I think this handles most of the concerns about speed, etc.

It does address the speed considerations, but part of what "unsupported" means is that the CLI team doesn't intend, itself, to enforce that the CLI's downstream dependencies continue to work on unsupported versions of Node. It would be useful advisory information for people submitting PRs, if they actually paid attention to build results, but in practice, they don't. (This is probably our fault, because it's turning out to be a pretty major effort to keep Travis green, and AppVeyor is still a work in progress, which in effect means that you have to be on the core team to be able to interpret what our CI is telling you.) So in effect, it's not information the core team officially cares about, so I'm not sure what value it provides.

Re: standard, if we wanted to get really serious about checking for ES6isms, we could fork it and make a the-old-standard or something, but that sorta sounds like more work than it's worth, given that it's intended to address a transition period of (hopefully) fixed duration.

I think, for now, I'm gonna ship this patch as-is, and then we can tweak from here. We're serious about being willing to merge contributions to patch any backwards compatibility regressions that sneak in, and you're right, we do review every contribution that gets merged in (even if that does (frequently) miss things). That will probably be enough for now, and if it turns out that people start running into problems, we'll have a better idea of what needs to happen then.

Thanks a ton for your feedback! <3

We still love u, 0.10, but we gotta keep up with the times. This is
_not_ a license to go wild with ES6isms. npm still _works_ (most of the
time) back to 0.8, and that's in large part because of the project's
conservatism about sticking to plain ES5 in both its own code base and
its npm-controlled dependencies. Dependencies not under the team's
control are already hard-deprecating 0.10, though, and the build matrix
already leads to long Travis times.

Credit: @othiym23
Blame: @othiym23
PR-URL: #14503
Reviewed-By: @zkat
Reviewed-By: @iarna
End of an era.

Credit: @othiym23
Reviewed-By: @zkat
Reviewed-By: @iarna
PR-URL: #14503
@othiym23 othiym23 merged commit 731ae52 into release-next Nov 3, 2016
@othiym23 othiym23 deleted the othiym23/rip-off-the-bandage branch November 3, 2016 21:43
@phated
Copy link
Contributor

phated commented Nov 3, 2016

Thanks @othiym23

part of what "unsupported" means is that the CLI team doesn't intend, itself, to enforce that the CLI's downstream dependencies continue to work on unsupported versions of Node.

I was thinking that the allow_failures section would be documented as such. Having a travis run to look at is much easier for community members, like myself, to glance at and see that a dependency is broken and work to fix it.

we could fork it and make a the-old-standard or something

What about using an older semver major that only supported ES5? I'm pretty sure there was a version like that.

othiym23 added a commit that referenced this pull request Nov 4, 2016
We still love u, 0.10, but we gotta keep up with the times. This is
_not_ a license to go wild with ES6isms. npm still _works_ (most of the
time) back to 0.8, and that's in large part because of the project's
conservatism about sticking to plain ES5 in both its own code base and
its npm-controlled dependencies. Dependencies not under the team's
control are already hard-deprecating 0.10, though, and the build matrix
already leads to long Travis times.

Credit: @othiym23
Blame: @othiym23
PR-URL: #14503
Reviewed-By: @zkat
Reviewed-By: @iarna
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants