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

Upgrade Node to 18 #6740

Merged
merged 39 commits into from
May 23, 2023
Merged

Upgrade Node to 18 #6740

merged 39 commits into from
May 23, 2023

Conversation

sujithvn
Copy link
Contributor

@sujithvn sujithvn commented Jan 4, 2023

Resolves #6396
Impact: major
Type: feature|bugfix

Issue

Current version is 14. Upgrade to 18.10.0

Solution

Primarily addressed the changes related to 'assert json' for json imports. All json imports are updated to have have assert { type: "json" }.
Additional config updates in babel, eslint and docker files (please refer the files changed)
Jest had to be upgraded from v25.5.4 to v29.4.3 to include fixes related to workerIdleMemoryLimit
Jasmine2 had to be explicitly installed while performing Jest upgrade

Breaking changes

Node required version upgraded from 14 to 18.10.0

Testing

Unit tests and Integration:query tests passed locally.
Integration:mutation failed in a Mac M1 Pro 32GB system, but passed in a Linux system with similar config.
All test passed in CircleCI after upgrade to LARGE configuration
Also successfully placed order using old storefront with updated version.

@changeset-bot
Copy link

changeset-bot bot commented Jan 4, 2023

🦋 Changeset detected

Latest commit: b309747

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 41 packages
Name Type
@reactioncommerce/api-core Major
@reactioncommerce/api-plugin-accounts Major
@reactioncommerce/api-plugin-address-validation Major
@reactioncommerce/api-plugin-address-validation-test Major
@reactioncommerce/api-plugin-authentication Major
@reactioncommerce/api-plugin-authorization-simple Major
@reactioncommerce/api-plugin-carts Major
@reactioncommerce/api-plugin-catalogs Major
@reactioncommerce/api-plugin-discounts Major
@reactioncommerce/api-plugin-discounts-codes Major
@reactioncommerce/api-plugin-email Major
@reactioncommerce/api-plugin-email-smtp Major
@reactioncommerce/api-plugin-email-templates Major
@reactioncommerce/api-plugin-files Major
@reactioncommerce/api-plugin-i18n Major
@reactioncommerce/api-plugin-inventory Major
@reactioncommerce/api-plugin-inventory-simple Major
@reactioncommerce/api-plugin-job-queue Major
@reactioncommerce/api-plugin-navigation Major
@reactioncommerce/api-plugin-notifications Major
@reactioncommerce/api-plugin-orders Major
@reactioncommerce/api-plugin-payments Major
@reactioncommerce/api-plugin-payments-example Major
@reactioncommerce/api-plugin-payments-stripe-sca Major
@reactioncommerce/api-plugin-pricing-simple Major
@reactioncommerce/api-plugin-products Major
@reactioncommerce/api-plugin-sample-data Major
@reactioncommerce/api-plugin-settings Major
@reactioncommerce/api-plugin-shipments Major
@reactioncommerce/api-plugin-shipments-flat-rate Major
@reactioncommerce/api-plugin-shops Major
@reactioncommerce/api-plugin-simple-schema Major
@reactioncommerce/api-plugin-sitemap-generator Major
@reactioncommerce/api-plugin-surcharges Major
@reactioncommerce/api-plugin-system-information Major
@reactioncommerce/api-plugin-tags Major
@reactioncommerce/api-plugin-taxes Major
@reactioncommerce/api-plugin-taxes-flat-rate Major
@reactioncommerce/api-plugin-translations Major
@reactioncommerce/api-utils Major
reaction Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@sujithvn sujithvn marked this pull request as draft January 11, 2023 17:01
sujithvn and others added 18 commits January 19, 2023 15:19
Signed-off-by: Brent Hoover <[email protected]>

Signed-off-by: Brent Hoover <[email protected]>
Signed-off-by: Brent Hoover <[email protected]>

Signed-off-by: Brent Hoover <[email protected]>
Signed-off-by: Brent Hoover <[email protected]>

Signed-off-by: Brent Hoover <[email protected]>
Signed-off-by: Brent Hoover <[email protected]>

Signed-off-by: Brent Hoover <[email protected]>
Signed-off-by: Brent Hoover <[email protected]>

Signed-off-by: Brent Hoover <[email protected]>
Signed-off-by: Brent Hoover <[email protected]>

Signed-off-by: Brent Hoover <[email protected]>
Signed-off-by: Brent Hoover <[email protected]>

Signed-off-by: Brent Hoover <[email protected]>
Signed-off-by: Brent Hoover <[email protected]>

Signed-off-by: Brent Hoover <[email protected]>
Signed-off-by: Brent Hoover <[email protected]>

Signed-off-by: Brent Hoover <[email protected]>
Signed-off-by: Brent Hoover <[email protected]>

Signed-off-by: Brent Hoover <[email protected]>
Signed-off-by: Brent Hoover <[email protected]>

Signed-off-by: Brent Hoover <[email protected]>
@sujithvn sujithvn marked this pull request as ready for review February 27, 2023 17:45
Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

  • Base branch needs to be release-5
  • needs changesets for all plugins
  • let's make the require/import thing consistent across all plugins

Otherwise seems ok

package.json Outdated
"npm": ">=7"
},
"engineStrict": true,
"scripts": {
"start:dev": "npm run start:dev -w apps/reaction",
"start:meteor-blaze-app": "npm run start -w=apps/meteor-blaze-app",
"build:packages": "pnpm -r run build",
"test": "pnpm -r run test",
"test": "pnpm -r run test --forceExit",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the right place to put this parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the parameter was correctly passed to jest as we could see in the earlier successful unit-tests (ex test-unit (79061)).

But thinking about it now, this was one of the option suggested when we had the unit-test stalling with jest v25. Now after the upgrade, the tests are passing without this param. Hence I have removed the same and all tests passed.

@tedraykov
Copy link
Collaborator

I think the changeset we should add to each plugin should be major because a plugin without the assert json part won't work with reaction 5 and vice versa.

What do you mean by require/import being consistent? Do we have plugins that use require?

@brent-hoover
Copy link
Collaborator

Yes, we have many. It was the other method we used for reading in the json files

@sujithvn
Copy link
Contributor Author

sujithvn commented Mar 2, 2023

@zenweasel @tedraykov

Regarding the changeset, I selected all plpugins for Major bump, but was not sure if we could assign Major bump for Reaction4. I assume this could change Reaction to v5 with the merge of this branch to release5 branch and create confusion. Hence I have not yet applied the changeset. I shall discuss tomorrow morning.

Regarding the require to import change, my understanding is that we should be making the change from require -> import only for .json file imports (by including the assert). I found only two files which has this condition and have updated that. (Currently eslint failed, but will fix it along with changeset changes). Rest of the 'require' calls are for few other external modules and files like config.cjs. I assume we need not update those.

@sujithvn sujithvn changed the base branch from trunk to release-5 March 3, 2023 10:20
@aldeed
Copy link
Contributor

aldeed commented Mar 3, 2023

External requires can be changed to import only if that package publishes an ESM entry point. It's becoming increasingly common, but still a lot of packages don't. I'd say that's a separate effort, but worth keeping an eye on the release notes of packages as you update them, in case they now include ESM.

apps/reaction/src/checkNodeVersion.cjs Outdated Show resolved Hide resolved
@sujithvn sujithvn requested a review from brent-hoover April 20, 2023 05:08
@sujithvn
Copy link
Contributor Author

  • Base branch needs to be release-5
  • needs changesets for all plugins
  • let's make the require/import thing consistent across all plugins

Otherwise seems ok

@sujithvn sujithvn merged commit 91a455f into release-5 May 23, 2023
@github-actions github-actions bot mentioned this pull request Jun 13, 2023
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 this pull request may close these issues.

Move to Node 18
5 participants