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

Fix NPM dependencies #1012

Merged
merged 9 commits into from
Feb 28, 2023
Merged

Fix NPM dependencies #1012

merged 9 commits into from
Feb 28, 2023

Conversation

adamraya
Copy link
Collaborator

@adamraya adamraya commented Feb 28, 2023

Description

The PR uses NPM 8 to re-generate the lock files using lockfileVersion:2 which is backwards compatible with lockfileVersion:1 solving the NPM warning npm WARN old lockfile.

Regenerating lock files using NPM 8 uncovered a few problems with the dependencies resolution in our package.json files.

These are the changes done to successfully re-generate lock files using NPM 7 or 8:

Package template-retail-react-app

Package pwa-kit-react-sdk

  • Remove enzyme-adapter-react-16. Since requires react@16 and we currently use react@17.

Generator script

After re-generating the package-lock.json files, the generator script was stuck waiting for the local NPM Verdaccio.
A recent change made in the integration hooks branch on a PR also regenerating lock files seems to solve that problem here as well.


The PR also fixes a Warning when pushing a bundle to MRT using Node 16.
The change is backwards compatible with Node 14.

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

How to Test-Drive This PR

Verify we can re-generate lock files using NPM 8.

  1. Checkout this branch fix-npm-peer-dependencies. Remove node_modules folders and package-lock.json files
git clean -dxf && find . -name 'package-lock.json' -type f -delete
  1. Switch to NPM 7 or NPM 8
nvm use 16.19
  1. Install dependencies
npm install
  1. Confirm lock files are being generated.

Verify we don't see a warning when pushing a bundle to MRT using Node 16.

  1. Using Node 16, run the npm run push command to push a bundle to an MRT environment.
  2. Verify there're no Node 16 warnings in the terminal.

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@adamraya adamraya changed the title Fix NPM peer dependencies Fix NPM dependencies Feb 28, 2023
@adamraya adamraya marked this pull request as ready for review February 28, 2023 08:28
@adamraya adamraya requested a review from a team as a code owner February 28, 2023 08:28
@@ -62,7 +62,6 @@
"@loadable/component": "^5.15.0",
"@wojtekmaj/enzyme-adapter-react-17": "^0.6.6",
"enzyme": "^3.8.0",
"enzyme-adapter-react-16": "1.15.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting that we didn't clean this up before. Nice catch

@bendvc
Copy link
Collaborator

bendvc commented Feb 28, 2023

Hey @adamraya, Back to the original question, which was what happens how when a dev runs npm ci when they are using npm 6? I think we wanted to know if those users were going to see a warning about the package-lock files having to be re-created or would npm6 know how to use those files created with npm 8.

@adamraya
Copy link
Collaborator Author

Hey @adamraya, Back to the original question, which was what happens how when a dev runs npm ci when they are using npm 6? I think we wanted to know if those users were going to see a warning about the package-lock files having to be re-created or would npm6 know how to use those files created with npm 8.

The generated lock files using NPM 8 use lockfileVersion:2 and are backwards compatible with lockfileVersion:1
https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json#lockfileversion:~:text=The%20lockfile%20version%20used%20by%20npm%20v7%2C%20which%20is%20backwards%20compatible%20to%20v1%20lockfiles

I confirmed this by removing the node_modules folders and running npm ci using NPM 6.

Copy link
Collaborator

@bfeister bfeister left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for fixing @adamraya 🙏

LGTM

@adamraya adamraya merged commit d2a49bc into develop Feb 28, 2023
@adamraya adamraya deleted the fix-npm-peer-dependencies branch February 28, 2023 22:34
bfeister added a commit that referenced this pull request Mar 9, 2023
* develop:
  Allow query hook parameters to be `null`. (#1046)
  Implement `updateCustomerPassword` as no-op. (#1031)
  Update Retail React App Page Designer integration README (#1041)
  BUG: Changed type of the phone number field to bring up numberic keyboard on mobile devices - W-9871940 (#1016)
  Move the MRT reference app to the SDKs, so that we can verify eg. Node support (#966)
  Update `develop` with `release-v2.7.0` (#1033)
  Remove unused dependencies. (#1020)
  Make some style decisions on padding (#1023)
  Update CHANGELOG.md (#1021)
  Support product-sets in retail-react-app (#1019)
  Simplify hooks logic (#959)
  Fix NPM dependencies (#1012)

# Conflicts:
#	lerna.json
#	package-lock.json
#	package.json
#	packages/commerce-sdk-react/CHANGELOG.md
#	packages/commerce-sdk-react/package-lock.json
#	packages/commerce-sdk-react/package.json
#	packages/internal-lib-build/package-lock.json
#	packages/internal-lib-build/package.json
#	packages/pwa-kit-create-app/package-lock.json
#	packages/pwa-kit-create-app/package.json
#	packages/pwa-kit-dev/CHANGELOG.md
#	packages/pwa-kit-dev/package-lock.json
#	packages/pwa-kit-dev/package.json
#	packages/pwa-kit-dev/src/configs/webpack/config.js
#	packages/pwa-kit-dev/src/ssr/server/build-dev-server.js
#	packages/pwa-kit-react-sdk/CHANGELOG.md
#	packages/pwa-kit-react-sdk/package-lock.json
#	packages/pwa-kit-react-sdk/package.json
#	packages/pwa-kit-runtime/CHANGELOG.md
#	packages/pwa-kit-runtime/package-lock.json
#	packages/pwa-kit-runtime/package.json
#	packages/template-express-minimal/package-lock.json
#	packages/template-express-minimal/package.json
#	packages/template-retail-react-app/app/hooks/use-add-to-cart-modal.js
#	packages/template-retail-react-app/app/partials/product-view/index.jsx
#	packages/template-retail-react-app/package-lock.json
#	packages/template-retail-react-app/package.json
#	packages/template-typescript-minimal/package-lock.json
#	packages/template-typescript-minimal/package.json
#	packages/test-commerce-sdk-react/package-lock.json
#	packages/test-commerce-sdk-react/package.json
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.

4 participants