Skip to content
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

Discussion to avoid node version incompatibility with LTSs #19050

Closed
cvializ opened this issue Oct 30, 2018 · 16 comments · Fixed by #19159
Closed

Discussion to avoid node version incompatibility with LTSs #19050

cvializ opened this issue Oct 30, 2018 · 16 comments · Fixed by #19159

Comments

@cvializ
Copy link
Contributor

cvializ commented Oct 30, 2018

We want to avoid needing to transpile features changing between LTS versions. But we also want to support the lowest common denominator until we decide to migrate off of ^8.0.0.

@jridgewell @kristoferbaxter

@cvializ cvializ changed the title Discussion to avoid node LTS incompatibility Discussion to avoid node version incompatibility with LTSs Oct 30, 2018
@kristoferbaxter
Copy link
Contributor

A few notes to help start the conversation:

We agreed to use the latest LTS version of Node to avoid supporting multiple Node.js releases with features that may or may not be compatible with transpilation.

Some of the initial motivation regarded the use of async/await, which was fully supported with the Carbon (8.x) branch of Node.js but unsupported in Boron (6.x). Both of these versions were active LTS at the same time, which meant we would need to compile our build system Node scripts using async/await or wait until April 2019 to start using the newer syntax officially supported starting October 2017.

Simultaneous LTS branches will be the new normal.

This issue will continue to impact projects like AMP while there are several active LTS branches of Node. There are three active LTS branches with Dubnium entering active LTS support today. If we choose to support multiple LTS branches we will need to ensure all build system code complies with the lowest supported LTS version.

screen shot 2018-10-30 at 2 08 32 pm

According to the schedule, there will be three active LTS branches from now until at least April 2022.

New LTS Releases can cause instability in the Project

  • Since we do not pro-actively test with newer versions of Node.js, it's possible an LTS release will cause the CI/CD pipeline to fall over.

  • Developers working on the project will be forced to upgrade at LTS release time, not on a schedule communicated by the AMP team.

  • Releases can be delayed if nvm doesn't detect the correct version when a LTS release occurs.

@jridgewell
Copy link
Contributor

I'm ok with abandoning v6. It's no longer getting updates.

But abandoning v8 just because v10 became the active LTS doesn't make good sense. We're just cutting off potential users.

@kristoferbaxter
Copy link
Contributor

There is definitely a tradeoff. Supporting multiple versions means we need to ensure (via travis) that each commit works against the supported versions. When debugging an issue it will be important to know which versions are impacted. Developers working on the AMP codebase will likely need to use nvm or similar to test changes on multiple versions of Node (especially if leveraging newer APIs).

As code is changing, we should have a strong preference around the usage of Node features if we support multiple versions of Node. Should all build code be transpiled to ensure it runs against the oldest supported version?

Because of these factors, I'd ask we support a single Node version going forward, and only one at a time. "lts/*" meets this criteria.

But abandoning v8 just because v10 became the active LTS doesn't make good sense. We're just cutting off potential users.

Let's see if any of the AMP design principles might assist in an answer.

User Experience > Developer Experience > Ease of Implementation.

This issue only impacts the ease of implementation, since developers of author pages shouldn't need to run the repository to test their documents. As a result, its by far the lowest priority for informing a design decision.

Don’t design for a hypothetical faster future browser.

No impact.

Don’t break the web.

Potentially supporting only a single node version reduces the time for a Travis build to complete, but they are run mostly in parallel (so the impact isn't that significant). If the version of AMP that is deployed is broken, pushing a fix could be slower. However, we also would likely rollback before pushing a fix – likely no impact.

Solve problems on the right layer.

No impact.

Only do things if they can be made fast.

No significant impact, newer versions of Node incorporate later versions of V8 which should improve overall build performance. However, the delta is likely low enough to be immaterial (metrics would help inform).

Prioritise things that improve the user experience – but compromise when needed.

No impact.

No "nicelists" or "allowed lists".

No impact since this relates to allowlists for special treatment of domains.

@torch2424
Copy link
Contributor

Thank you @kristoferbaxter for the detailed writeup! 👍

I like a lot of the points made, and agree it may be a good idea to only support one node version. Supporting multiple keeps coming up with these problems that we are facing, and have a hard time dealing with. However, this leads to a few issues:

  1. Anyone new to AMP or Node will already have a bit of a task getting node installed and running, and adding version requirements will make things just a bit more difficult than it could be.

  2. Veteran developers will have a the headache of trying to build AMP, and then realizing they need to switch node versions and get that all up and running before even start the project. Which, can be slightly annoying.

  3. We will need to make sure any documentation that references how to build or contribute to AMP mentions the exact node version we are currently supporting, and this needs to be updated along with any version change.

However, I have another idea. Can we soft-support the latest stable? As in, we claim to only support LTS, but to avoid the headache of having to scramble to fix things for the newest node versions when the LTS changes, add the stable to our current Travis build pipeline, and throw warnings whenever something is breaking on Stable. That way our team can start to think about and open issues on things that will break. And anyone jumping into the project with whatever node version should be able to start developing without any issues 9/10 times 😄 . Any issues of them using features in the newest node version will be caught by travis, and they can do the usually small amount of refactoring to get their code working. 👍

Let me know what you think!

cc @cvializ @jridgewell

@jpettitt
Copy link
Contributor

jpettitt commented Nov 1, 2018

Since LTS branches change once a year is it really that much of a burden to require the highest LTS version that's available? It's not hard to install a new version withnvm

@torch2424
Copy link
Contributor

@jpettitt

Yes, I agree LTS changes don't happen to often, and changing them once a year isn't too bad 😄

But I guess I am speaking more for the developer who may be new to development, or, a developer who doesn't work on AMP regularly. In both cases, it can be an extra step that can be annoying quickly if you are jumping around projects, or trying to understand why things aren't working. Though, I may just be nitpicking on their behalf.

Let me know what you think 😄

@jpettitt
Copy link
Contributor

jpettitt commented Nov 5, 2018

It sounds like what we need to do is change the requirement from LTS to a major version number. The version we select should always be in the LTS window and when a new LTS is available we should test it in the CI/CD chain and make a change to the tooling requirement on our schedule (but always before the current version EOL).

This still means devs have to update their toolchain so @torch2424's point about dev workload is still valid (although only once a year). That's unavoidable as we'll always need to deprecate node versions as they fall out of their maintenance window.

@kristoferbaxter
Copy link
Contributor

The issue with pinning to an exact version is delayed upgrades via manual enforcement. Although infrequent (~ once a year), the delay in upgrading can be removed entirely by using lts/*.

Downsides:

  1. Version changes automatically, without notice, for all developers.
  2. Uncertainty regarding stability of lts/*, since codebase isn't tested before the lts release is required for all developers. (note: this is an issue needing resolution independently)

Upsides:

  1. No manual effort to move Node versions
  2. Can use the latest V8 features (and Node APIs) with confidence to make our build system code more readable, testable, and performant.

@jpettitt
Copy link
Contributor

jpettitt commented Nov 6, 2018

I was suggesting pinning to a major version. eg ^8.0 to ^10.0 that should keep us up to date with bug fixes bug avid major version breakage.

Another approach would be to let greenkeeper do it on the basis that if it tries to update no in a way that breaks the test will catch it and it won't get merged.

@alanorozco
Copy link
Member

driving by 🚗

I would argue that "we support LTS" is a much clearer message than "we support X and Y". It's also easier to develop against, since the only contextual information needed is that support for LTS is available and so are its features.

The hurdle of switching Node versions should be just as frequent regardless of the versions we choose to support, assuming we're trying to keep a modern codebase.

Automating version switch (e.g. testing) is a problem regardless of the decision taken here as mentioned by @kristoferbaxter. However, by only supporting LTS we can fully automate the process, including the trigger (i.e. LTS version switch).

@ampprojectbot ampprojectbot added this to the Pending Triage milestone Nov 8, 2018
@jridgewell
Copy link
Contributor

Supporting multiple versions means we need to ensure (via travis) that each commit works against the supported versions.

This is what Travis is for.

When debugging an issue it will be important to know which versions are impacted.

I think this is minor issue. As far as I'm aware, we've only had one issue with using a v8 feature when we were still supporting v6.

Developers working on the AMP codebase will likely need to use nvm or similar to test changes on multiple versions of Node (especially if leveraging newer APIs).

The infrastructure team, only. Everyone else will be fine with whatever they have installed during their setup.

As code is changing, we should have a strong preference around the usage of Node features if we support multiple versions of Node. Should all build code be transpiled to ensure it runs against the oldest supported version?

I think you're overstating the speed at which normal devs adopt new features, and the speed at which features are added to node. This is the build side of the project, not the actual code we're trying to ship to users. Note that Babel itself doesn't try to use the latest features, it complies down to whatever is appropriate.

Because of these factors, I'd ask we support a single Node version going forward, and only one at a time. "lts/*" meets this criteria.

@rsimha might be able to respond with issues that are raised every time we node releases a new version. Every time, one of the native build dependencies breaks because it wasn't updated to the new native APIs. Using a rolling, non-pinned major target is a terrible decision, regardless of our v8 vs v10 debate.

It's not hard to install a new version with nvm

Devs don't always control the version. For instance, I had a discussion with the GCP devs about how they were still locked to v6 earlier this year.

But for normal devs on their own machines, I think nvm is an easy enough thing to install the necessary version.


We don't gain any features by forcing v10 as the minimum version. The release summary for v9 and v10 doesn't have any necessary features node features. Even looking at node.green, the only language changes I can see are Shared arrays, Promise.p.finally, newer regex features, async iterators, and BigInt. Nothing in that is critical for new tooling development, and we're not even using those features in actual codebase.

@rsimha
Copy link
Contributor

rsimha commented Nov 9, 2018

Thanks for starting this discussion.

To recap, The 10.0 LTS release broke us because we pinned our project's package.json to the latest major LTS version (^8.0.0). This was okay when there was just one active LTS, although it still needed us to manually update the currently allowed version.

To fix this, the plan (after discussing with others) is to enable node version updates from Renovate using "supportPolicy": ["lts_active"] (https://renovatebot.com/docs/node/). This will allow us to use any one of the active LTS versions of node for local development, while using the latest active LTS for testing on Travis.

Edit: We'd also need to revert the version specified in .travis.yml to lts/*. This is being done in # #19234

@davidstrauss
Copy link
Contributor

This will allow us to use any one of the active LTS versions of node for local development, while using the latest active LTS for testing on Travis.

Would this mean that Travis would not test against older (but still active) LTS releases?

@rsimha
Copy link
Contributor

rsimha commented Nov 12, 2018

Would this mean that Travis would not test against older (but still active) LTS releases?

Correct. As configured today, Travis will only test against the latest active LTS.

In order to run all tests on Travis against the latest LTS, we'd need to double our VM usage. The argument could be made for merely making sure the runtime builds correctly on older active LTS version(s), but I'm not sure if that's a large enough benefit, given that we expect a subset of development to be done using those versions.

@rsimha
Copy link
Contributor

rsimha commented Jan 8, 2019

Update: According to https://github.com/nodejs/Release, node 8 has entered Maintenance LTS status. The current active LTS is node 10.

I'll send out a PR.

@rsimha
Copy link
Contributor

rsimha commented Oct 24, 2019

One year later, we've run into similar problems once again, due to node's latest active LTS going from 10 to 12. The latest discussion on this topic can be found at #25209. (Bumping this old issue to link the two.)

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

Successfully merging a pull request may close this issue.

9 participants