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

Drop Node 14 support, add Node 20 #16557

Merged
merged 31 commits into from
Jul 24, 2023
Merged

Conversation

innerdvations
Copy link
Contributor

@innerdvations innerdvations commented Apr 28, 2023

What does it do?

Removes support for node 14 and adds node 20.

Why is it needed?

Node 14 is EOL (including security fixes) and Node 20 has been released.

How to test it?

Everything should work when using Node 20 and Strapi should no longer start with Node 14

Related issue(s)/PR(s)

Blocking #16849

Documentation PR

@innerdvations innerdvations requested review from Convly, derrickmehaffy and alexandrebodin and removed request for derrickmehaffy April 28, 2023 09:32
@innerdvations innerdvations added the flag: don't merge This PR should not be merged at the moment label Apr 28, 2023
@innerdvations innerdvations changed the title Remove Node 14 support, add Node 20 [POC] Remove Node 14 support, add Node 20 Apr 28, 2023
@innerdvations innerdvations changed the title [POC] Remove Node 14 support, add Node 20 [POC] Drop Node 14 support, add Node 20 Apr 28, 2023
Copy link
Member

@derrickmehaffy derrickmehaffy left a comment

Choose a reason for hiding this comment

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

I wasn't planning on updating all the engines just wanted to enable the tests but why not :P

examples/kitchensink-ts/package.json Outdated Show resolved Hide resolved
examples/getstarted/package.json Outdated Show resolved Hide resolved
examples/kitchensink/package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/cli/create-strapi-app/package.json Outdated Show resolved Hide resolved
packages/providers/upload-aws-s3/package.json Outdated Show resolved Hide resolved
packages/providers/upload-cloudinary/package.json Outdated Show resolved Hide resolved
packages/providers/upload-local/package.json Outdated Show resolved Hide resolved
packages/utils/logger/package.json Outdated Show resolved Hide resolved
packages/utils/typescript/package.json Outdated Show resolved Hide resolved
@innerdvations
Copy link
Contributor Author

innerdvations commented May 3, 2023

I wasn't planning on updating all the engines just wanted to enable the tests but why not :P

I decided against updating the lower requirement at this point to keep the impact of your PR minimal, and only "officially" remove support but allow it to work until we can get more discussion on it. It felt weird to me to arbitrarily break a version of node in a minor version release.

Update: We decided to remove Node 14 from engines to avoid potential issues if we use functions that are not available in node 14.

alexandrebodin
alexandrebodin previously approved these changes Jul 1, 2023
@innerdvations
Copy link
Contributor Author

It appears Node 20 with mysql 5 in the CI (I can't reproduce locally) returns unsorted results from the API in a different order than other cases. I consider that a bug with the test and will fix it everywhere it occurs, but I want to confirm with @alexandrebodin that users should not expect results from the API to come in any specific order unless explicitly sorted in the query params.

Copy link
Member

@Convly Convly left a comment

Choose a reason for hiding this comment

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

users should not expect results from the API to come in any specific order unless explicitly sorted in the query params

That should definitely be the expected behavior 💯
I would say we can go ahead with this, but we can also wait for @alexandrebodin approval if you prefer.

@alexandrebodin
Copy link
Member

@innerdvations I can confirm about the sorting. Just very surprising that it occurs for node 20. A bit worrying. I'll let you double check this can't come from a known node 20 change and move forward with @Convly

@strapi-bot
Copy link

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/strapi-with-typescript-option-and-docker-postgres-image-typeerror-reading-routes/30982/3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: chore This PR contains chore tasks (cleanups, configs, tooling...) source: core:strapi Source is core/strapi package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants