Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Raise requirement from Node 0.12 to Node 4? #76

Closed
felixfbecker opened this issue Apr 12, 2017 · 26 comments
Closed

Raise requirement from Node 0.12 to Node 4? #76

felixfbecker opened this issue Apr 12, 2017 · 26 comments

Comments

@felixfbecker
Copy link
Contributor

Node 0.12 entered maintenance on 2015-10-01 and reached end of life on 2016-10-31.
Latest Node version is v7, current LTS is v6 and v4 is LTS until 2018. Many libraries dropped support for Node 0.12, for example TSLint, preventing the usage for us. I think moving to Node 4 and following the LTS schedule would be reasonable.

@tedsuo
Copy link
Member

tedsuo commented Apr 17, 2017

Following the LTS schedule makes sense, though I'd prefer we favor hanging back even farther until being forced to upgrade. The node LTS schedule is only 30 months long, which sounds long but is actually quite short.

v4 is the first LTS version of node to be offered; that's a good starting point. I'm guessing we would not be abandoning anyone if we bumped up to it.

Also, we should probably document this policy explicitly.

@tedsuo
Copy link
Member

tedsuo commented Apr 17, 2017

To be clear though, we need to at least poll the community on gittter and have a response window available before making the change.

@yurishkuro
Copy link
Member

What is it going to entail? A lot of business critical services at my company are on node 0.10

@felixfbecker
Copy link
Contributor Author

felixfbecker commented Apr 17, 2017

I mean, this is only the support for the latest version, which means the ones that are actively verified to work on Travis. You can always depend on an older version of opentracing-javascript if you need support for an older Node version, they will always be available on npm, and people can even backport important bug fixes if there is the need.

@felixfbecker
Copy link
Contributor Author

This is what most packages (e.g. lodash, sequelize) do

@felixfbecker
Copy link
Contributor Author

@yurishkuro Currently Node 0.12 is what is tested against.

@yurishkuro
Copy link
Member

I am not sure what that means. Aren't all the tests written in Typescript now?

My main concern is for the final artifacts in npm to be usable in older Node versions. I don't care if we use higher version for transpiling, but if we drop older Node versions support from the artifacts then we need some other huge win by upgrading. Note that in Java API we went as far back as Java 6, because people still run that in prod. Given that the artifacts we produce are really just an interface and Noop implementation, I don't think upgrading lodash, sequelize, etc. is a strong argument.

@bhs
Copy link
Contributor

bhs commented Apr 17, 2017

@yurishkuro someone reminded me that Uber has dependencies on 0.10... is that correct?

@felixfbecker
Copy link
Contributor Author

What exactly do you mean with "the final artifacts in npm"? The distributed package of the latest version? Or of all versions?

Usually, what versions are supported is denoted by the Node versions you test against in CI. That is not possible with Node 0.12 because the tools like TypeScript itself, TSLint etc are following the Node LTS schedule and only support Node >4. So at least on Travis, we can only test against >4.

My proposal was to officially only support these versions then. What would be the real benefit of releasing a 1.0 still with Node 0.12 support? The major feature is the TypeScript support, which you won't be able to use if you are on Node 0.12, because older TypeScript versions that still support 0.12 won't be able to understand the new syntaxes used. If you use only JavaScript, then there is no reason for you to upgrade to 1.x anyway.

@tedsuo
Copy link
Member

tedsuo commented Apr 17, 2017

Hmmm. If Uber is still on 0.10, I suspect a lot of companies are. I don't like dropping support for a version of node that the community is still using. And I don't like telling people to use outdated versions of the OT API, especially if we aren't committed to patching or maintaining them. The OT spec is supposed to represent a universal API that can be used across all platforms, that should include all runtime versions.

Especially because the OT API itself does not depend on any new features or have problems maintaining backwards compatibility, and it's only our developer tools that are creating a problem, is there another approach we can take? For example, peg at a version of TSLint that still supports v0.10, and restrict ourselves to that subset of TypeScript? I get that TypeScript is a core tool, and we should retain it if we can, but should it really override other priorities? It feels like what versions we support should dictate what tools we get to use, not the other way around. For example, we support Java 6 even though that is annoying and limits some of our choices.

@felixfbecker
Copy link
Contributor Author

felixfbecker commented Apr 17, 2017

This is only about the versions that are tested on Travis. I don't see a reason why the compiled Javascript would not work with 0.12 or 0.10. Note that even right now, 0.10 is not tested on Travis, only 0.12. So Uber's Node version is already not tested. Not even npm itself tests on Node 0.12. Babel doesn't either, so even before my TS PR was the 0.12 support possible to break on any new release of Babel, without any notification. Neither webpack, which was also used already before my PR.

Do we really have to test the functionality of an interface+noop package on a Node version that doesn't even get security fixes (OpenSSL won't be getting patched)? If someone is okay with running that, shouldn't he accept that he may need to run some packages at own risk?
There is nothing stopping someone to backport fixes to these versions if needed, but what kind of fixes would that be for an interface+noop implementation? What do we need to test differently on 0.12 than v4?

If the ecosystem never moves to the current versions because some haven't upgraded yet, then there will never be a reason for those to upgrade. Imo we should not block new features (like TypeScript in general) for new users just because we cannot run CI on them on an ancient Node version.

If Uber needs at all cost the latest OTjs 1.x, can absolutely not upgrade to Node 4 and wants its version to be verified in a public CI, then the only solution would be to install a node version manager in Travis, compile and lint with Node 4, then switch node version to an env var and run the tests (which, btw, do nothing but assert that some functions exist). Is this really needed?

@tedsuo
Copy link
Member

tedsuo commented Apr 18, 2017

Ok, I was confused. You are interested in raising the required node version for running the development environment. I think that is totally great. 👍

As far as having test coverage for earlier versions of node... yeah, I think you are correct. Since opentracing-javascript itself does not have any dependencies, only devDependencies, there is no need to test against any particular version of node, as there is no reason it would break.

@yurishkuro
Copy link
Member

I disagree that there's nothing to test. There is the noop implementation that needs to be tested to ensure it satisfies the API, returns correct objects, does not throw exceptions. And there's the API harness that can be used to test other implementations for API compliance. Plus even the main API is defined as a delegate pattern, that can be tested. All of these should be transpiled into ES5 Javascript that can run in pre-4 versions.

End of life for 0.12 was just 6mo ago. According to this https://semaphoreci.com/blog/2016/10/12/nodejs-versions-used-in-commercial-projects-2016-edition.html, almost 50% of production deployments still use 0.10 and 0.12

@felixfbecker
Copy link
Contributor Author

@yurishkuro I went the extra mile and modified the travis.yml to build with Node 4 but test against 0.10, 0.12, 4, 6 and latest.

@bhs
Copy link
Contributor

bhs commented Apr 21, 2017

@yurishkuro are you ok with this getting merged?

@yurishkuro
Copy link
Member

yes, lgtm

@yurishkuro
Copy link
Member

I think we decided to maintain 0.10 and up, so I am closing this. Please reopen if needed.

@felixfbecker
Copy link
Contributor Author

I think it would be good to add an engines field to package.json and highlight that support with a badge

@yurishkuro
Copy link
Member

Can you do a PR for that?

@rochdev
Copy link
Contributor

rochdev commented Jun 22, 2018

Over a year later, would you reconsider bumping the minimum version? I'm trying to implement the scope manager right now and I have a solution that works from Node 4, but Node 0.x has been a real blocker so far.

@yurishkuro
Copy link
Member

This repo is written in Typescript. How is is preventing you from implementing scope managers?

@rochdev
Copy link
Contributor

rochdev commented Jun 23, 2018

What do you mean? The features available in each version of Node is unrelated to the transpiler being used by the project.

Basically my implementation relies on async_wrap for Node 4-6 and async_hooks for Node 8-10. There is a polyfill for these native features but it’s proven unstable. I don’t think it would be reasonable to prioritze supporting old versions that have been deprecated for years and have nearly no usage in the wild over improving this library with new features.

@yurishkuro
Copy link
Member

If there's a polyfill for old versions, I don't see the problem - whoever is still on 0.x won't use the scope manager, they are already passing the context in some way.

@rochdev
Copy link
Contributor

rochdev commented Jun 23, 2018

As I’ve mentioned, the polyfill is not stable. It loses context in some scenarios and has memory leaks. It also doesn’t support a destroy hook that is necessary to properly clean up the scopes.

What you’re proposing is to continue supporting 0.x without the scope manager and only support the scope manager in 4+? That would work. Would an error be thrown when trying to use the scope manager in 0.x?

@rochdev
Copy link
Contributor

rochdev commented Jun 23, 2018

When the memory leak issue is fixed, we could probably attempt to add support for Node 0.x with the polyfill. In the meantime I think throwing an error would be the best course of action.

@yurishkuro
Copy link
Member

I think it's perfectly fine to not support a feature for an old Node version. At least at Uber, Node 0.10 is still running, but is on decline as those backend services are being rewritten in Go, and most of our web frontends are on newer versions. I'm actually looking forward to having a way to pass the context implicitly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants