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

feat: require node 16 and higher #1136

Merged
merged 3 commits into from Mar 6, 2022
Merged

feat: require node 16 and higher #1136

merged 3 commits into from Mar 6, 2022

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented Mar 2, 2022

Node.js v16 or higher are now required to use MSW.

Roadmap

@kettanaito
Copy link
Member Author

kettanaito commented Mar 2, 2022

The interceptors release that bumps Node to 16 hasn't been completed yet:

https://github.com/mswjs/interceptors/actions/runs/1919238772

Semantic release behaves unpredictably and disregards the previously set version of Node.

@kettanaito kettanaito added the BREAKING CHANGE Pull request introducing breaking changes. label Mar 2, 2022
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 6, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 17e4d76:

Sandbox Source
MSW React Configuration
objective-cohen-l1khq0 Issue #1125
winter-field-yieklt Issue #1125

@kettanaito kettanaito marked this pull request as ready for review March 6, 2022 17:15
@kettanaito kettanaito merged commit 6f4895e into main Mar 6, 2022
@kettanaito kettanaito deleted the chore-node-16 branch March 6, 2022 17:21
@github-actions
Copy link

github-actions bot commented Mar 7, 2022

🎉 This PR is included in version 0.39.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@thedavidprice
Copy link

@kettanaito This PR (via v0.39.0) blocks us from upgrading MSW in RedwoodJS packages due to the bump of Node requirement to v16. Redwood's engine requirements are >= 14.17 <= 16, and will include 18 when the LTS is released in May.

Deployment providers, e.g. Vercel, do not all support > v14 at this time, which is what sets our minimum engine requirements because local dev needs to match the deployment build environment.

What's the best way to follow up with you about this for discussion? Assuming an Issue unless you direct otherwise.

MSW is amazing and powers our API test suite and built-in Storybook integration! It's an exciting technology and we're grateful for the work by you and the community of contributors – thank you!

cc @msutkowski

@kettanaito
Copy link
Member Author

Hey, @thedavidprice 👋

We value RedwoodJS as one of the early adopters of MSW and would like to make sure that our integration is smooth.

That being said, the required Node.js versions you've mentioned seem to satisfy what MSW has published in 0.19.0: v16 that we now require lies within the scope >= 14.17 <= 16.

We've noticed more and more users switching to v16 as 16.14.0 is now the LTS version to be supported for 2 more years. We used to test our entire toolchain on v12, which reaches the end of life in 2 months. I think it's sensible we migrate to the LTS version beforehand and focus on supporting it.

Internally, we do not rely on v16 features, only handle certain breaking changes that v16 introduced (like removal of the IncomingMessage.headers property and moving it to be a prototype setter/getter instead. You can use v0.39 with Node.js 12 with no issues, apart from the engines incompatibility error. The latter you can suppress via --ignore-engies, if needed.

Deployment providers, e.g. Vercel, do not all support > v14 at this time

Yes, I understand this chain of dependencies. Given that v14 reaches EOL next year, I think there's a high chance the Vercel team migrates to v16 this year.

What's the best way to follow up with you about this for discussion? Assuming an Issue unless you direct otherwise.

We can open a designated discussion on GitHub, or follow any related questions right here if you find it comfortable. I want MSW to be functional in RedwoodJS, and I also want it to power up interceptors for those who use the LTS version of Node.js. I'm open to more approachable updates/strategies in achieving this.

@MichaelDeBoey
Copy link

@kettanaito I can confirm that msw@0.39.0 will work on Node v12
remark-embedder/transformer-oembed#50

So maybe it's a good idea to loosen up the engines if we're not relying on anything specific?

@kettanaito
Copy link
Member Author

Thanks for looking into it, @MichaelDeBoey!

So maybe it's a good idea to loosen up the engines if we're not relying on anything specific?

I think we can do that. I went with explicit engines so people know what Node.js version is officially supported. I wish it produced a warning instead of an error. Otherwise, people will expect things to work on v12 but we are dropping official support for v12.

@MichaelDeBoey
Copy link

If you really want to support v12 & v14, engines are fine as-is
Otherwise, you could loosen them up for as long as you're not using Node-specific things

@kettanaito
Copy link
Member Author

If you really want to support v12 & v14, engines are fine as-is

No, there's no plan to support these versions of Node.js. I think the current value of engines does just that: it throws when you attempt to install msw in a project with Node.js version < 16:

msw/package.json

Lines 11 to 12 in ad42af6

"engines": {
"node": ">=16.x"

I thought your suggestion to "loosen" the engines meant the opposite: to add 12/14 or to remove the engines property altogether. I think I got it wrong.

Otherwise, you could loosen them up for as long as you're not using Node-specific things

Could you please show me an example of how do you see us loosing this up? Thanks!

@MichaelDeBoey
Copy link

Could you please show me an example of how do you see us loosing this up? Thanks!

I indeed meant to add 12 and/or 14 again in the engines field, like

"node": ">=12.x" 

But if that's not what we want to support anymore, than it's just seen as a coincidence that v12 & v14 are still working (for now) and that's fine too I think.

@kettanaito
Copy link
Member Author

Also an extremely peculiar find: npm warns on mismatched engines while Yarn throws an error.

npm WARN notsup Unsupported engine for msw@0.39.0: wanted: {"node":">=16.x"} (current: {"node":"14.18.1","npm":"6.14.15"})

npm still allows installing packages because engines was never meant to throw. There's the enginesStrict flag which controls whether engines mismatches should throw or not. Yarn should respect that but, apparently, it doesn't.

The engines were designed for this particular scenario: set the recommended versions of tools without any explicit statement whether mismatching versions are supported or not. Versions mismatches must never throw.

Does anybody have a chance to see how Yarn 2 treats engines?

@celcius112
Copy link

@kettanaito tested with Yarn 3 (still using node_modules) in an Angular project, no problem with Node 14.18.

@jtoar
Copy link

jtoar commented Mar 8, 2022

@kettanaito I found this discussion in the yarn discord:

image

Link: https://discord.com/channels/226791405589233664/889442263229403166/949657093005250631.

merceyz is a core contributor and has helped us (I work with @thedavidprice on RedwoodJS) many times.

@thedavidprice
Copy link

@kettanaito Thank you for continuing this discussion! It's greatly appreciated.

You can use v0.39 with Node.js 12 with no issues, apart from the engines incompatibility error. The latter you can suppress via --ignore-engies, if needed.

Unfortunately, that won't work for us as we have 100's (if not 1000's) of dependencies we have to consider when running functional tests. There have been packages that bumped engines (for feature/performance/bug reasons), which we need to catch in CI. (Side note: this will be the first dependency of any that is dropping support for Node.js v14.)

Example: here is the failing CI check that allowed us to see the engine bump for this package.

Could you please show me an example of how do you see us loosing this up?

Here's how we set the requirements for RedwoodJS projects (note: we will be bumping minimum to 14.19 for 1.0.0 release in a few weeks):

And then we set CI to run a matrix using GitHub Actions:

@kettanaito
Copy link
Member Author

I certainly don't mind setting a >=12.x range in engines if it makes others' lives easier. I somewhat dislike that we cannot treat engines as the description of what Node.js versions we officially support though. I'll leave this decision to you.

@kevcenteno
Copy link

Another thing to consider is bumping the major version when dropping support for node versions.

@kettanaito
Copy link
Member Author

@kevcenteno, MSW is pre 1.0, which means minor versions can and will contain breaking changes. You also won't get the next minor version installed automatically with ^0.36.0 for example. This has been released properly per semver.

@thedavidprice
Copy link

This has been released properly per semver.

💯And PR indicated clearly. Although I can see people being caught off guard because the Release Notes did not indicate anything was breaking (just a fwiw).

I somewhat dislike that we cannot treat engines as the description of what Node.js versions we officially support though.

Also very much agreed. And something we're having to deal with as Redwood moves to Yarn v3. We'll report back once we hear some suggestions/recommendations from the Yarn team.

I certainly don't mind setting a >=12.x range in engines if it makes others' lives easier.

I am asking here that MSW both support v14 and v16 as well as set the engines accordingly. And by "support" I specifically mean run a CI matrix for v14 and v16:

  1. workflows/ci.yml
  2. workflows/smoke.yml

It's likely there's missing context on my part, but at this time I understand the decision to be about preference to reduce the overhead of supporting anything other than v16.

I'd be happy to help and continue the discussion via a PR. Just let me know!

@kettanaito
Copy link
Member Author

Good. I think we have agreed on the next steps:

  1. Set the "engines": ">=14" in package.json.
  2. Use both 14 and 16 versions in the workflows to ensure support.

I can agree with n.2 only because there are little implementation differences that concern MSW between those Node.js versions. I'd still be proposing to drop v14 next year when it reaches EOL.

I will go forth with making these changes happen.

@thedavidprice
Copy link

I will go forth with making these changes happen.

Huge thank you @kettanaito Grateful for the spirit of this conversation as well 🚀

I'd still be proposing to drop v14 next year when it reaches EOL.
💯

We've had to navigate through similar decisions as well and even recently were trying to determine if we could limit support to only the current Node LTS version, but the challenge for Redwood is that we have to consider not just local dev but also deployment provider build environments, which are the true constraints. (Note: build environment is not the same for us a runtime, which we manage separately.)

One compromise we've made is to bump the requirements of the maintenance LTS (i.e. 14 in this case) to the lastest semver minor. So Redwood supports >= 14.19, which achieves the v14 requirement but doesn't require us to futz with any potential issues from earlier versions. (Plus, it just seems like good practice overall.)

And as much as I've wished we could limit to supporting only two versions of Node, we just won't be able to get around overlap between 14, 16, and 18 for a portion of this year... 😕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE Pull request introducing breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Axios never receives mocked response object msw@0.38.1 Headers is undefined when not handling request
6 participants