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

Breaking Change with Node 16 in Minor Release #810

Closed
mhogara opened this issue Jul 31, 2024 · 5 comments · May be fixed by #811
Closed

Breaking Change with Node 16 in Minor Release #810

mhogara opened this issue Jul 31, 2024 · 5 comments · May be fixed by #811

Comments

@mhogara
Copy link

mhogara commented Jul 31, 2024

Expected Behaviour
In the most recent release of testcontainers-node(https://github.com/testcontainers/testcontainers-node/releases/tag/v10.11.0), there is a change to stop using node-fetch in favor of the the native fetch first introduced in Node 18. This means this release will break anything running Node 16.

Overall, I would have expected this change to come in a major release or for there to be clear documentation saying that testcontainers-node needs >= version 18 or node to run. I couldn't find any references to that on this GitHub or in the documentation site(https://node.testcontainers.org/configuration/).

I will admit that Node 16 should not be used given it already reached EOL, but my team works across many machines with various versions of Node and it is hard for us to keep track of which versions have been updated. This request is for better warnings/release strategy to help us prevent seemingly random changes.

Please let me know if my understanding is incorrect here

Actual Behaviour
Using a careted version(^10.9.0) in the package.json means we automatically pulled in the latest version of testcontainers-go and were surprised to see our HTTP wait strategies all stop working. We were able to debug and see it all traced back to the fetch call.

Steps to Reproduce

  1. Using Node 16
  2. Write a program with testcontainers
  3. Use a HTTP wait strategy in the most recent version of testcontainers.
  4. See failure and timeout of wait strategy

Environment Information

  • Operating System: Debian 11
  • Docker Version: Docker version 20.10.5
  • Node version: 16
  • Testcontainers version: 10.11.0
@cristianrgreco
Copy link
Collaborator

cristianrgreco commented Jul 31, 2024

Thanks for raising @mhogara. I agree that if the release breaks support for a given node version that ideally it should be a major release. However I'm not sure how feasible that is in practice. Support for node 16 ended almost 2 years ago. The pipelines for this project test against the current LTS (20) + the previous LTS (18). I could include even the previous LTS (16) which would have caught this breaking change, but then someone would probably raise an issue saying I broke something with node 14. My point is I'm not sure how feasible it is for any maintainer to support X unsupported node versions. Let me know your thoughts

@kiview
Copy link
Member

kiview commented Aug 1, 2024

I think it makes no sense from the side of the project to support a version that is EOL 2 years ago.
However, I agree that in the future we should call out changes to minimum supported Node versions in release notes, assuming these were conscious decisions and we are aware of them.

In addition, I think it would also make sense to clearly document, that a minimum Node version 18 is required (basically the lower bound of what we have in CI).

@mhogara
Copy link
Author

mhogara commented Aug 1, 2024

Thank you for the quick response @cristianrgreco , @kiview. I agree with the sentiment here- as a TLDR: it's very sensible to not support EOL versions of Node, but documentation/some announcement of removal of Node support would be great.

I understand that maintaining/running tests against Node16 or any unsupported node version isn't a good use of time, especially given that the EOL isn't recent.

That being said, it would benefit the user to have a clear documentation on what versions of Node are supported or not. The issue here really isn't the fact that testcontainers-node doesn't work on Node16, it's the fact that it happened in a minor version and AFAIK there is not documentation/explicit mention in the release notes that this will happen. The best I could find is this PR: https://github.com/testcontainers/testcontainers-node/pull/675/files where it's taken out of the test runs. Personally, I'd be thrilled to see a minimum requirements page somewhere that lists explicitly what versions are needed of any dependencies. I'm working across a lot of environments, so it's a lot easier when I have something I can reference for weird errors.

@cristianrgreco
Copy link
Collaborator

cristianrgreco commented Aug 1, 2024

Going forwards I think we can do the following:

  1. Bump the node engine requirement from 10 (😅) to 18 and publish a major release.
  2. Ensure that our pipelines test against the minimum node engine requirement (currently is the case).
  3. Do not remove node versions as was done in Retire Node 16 (End-of-life) from CI #675 from our pipelines without updating the engine requirement.
  4. Changes to the node engine requirement require a major release.

I hear what you say in terms of documentation, but it wouldn't have helped in this case (you would have read that we support node 16 when you first added testcontainers-node, and then one day it stops working). Whereas following the steps above we'd ensure we follow semantic versioning and then this scenario wouldn't happen in the first place.

What do you think / anything else to add?

@mhogara
Copy link
Author

mhogara commented Aug 1, 2024

@cristianrgreco The plan you outlined sounds great! 👏🏼 I have no flags or additional comments.

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

Successfully merging a pull request may close this issue.

3 participants