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

@W-13846314@ Enforce lockfile version 2 #1389

Merged
merged 5 commits into from
Aug 3, 2023
Merged

@W-13846314@ Enforce lockfile version 2 #1389

merged 5 commits into from
Aug 3, 2023

Conversation

wjhsf
Copy link
Contributor

@wjhsf wjhsf commented Jul 28, 2023

@adamraya pointed out that in the last release v3 snuck in to a few lockfiles. Since we don't really pay attention to the lockfiles as much as we should, it seems prudent to have a script do that for us.

Description

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

Changes

  • (change1)

How to Test-Drive This PR

# Create a v3 lockfile in monorepo root and pwa-kit-dev
git switch develop
rm -rf node_modules package-lock.json packages/pwa-kit-dev/node_modules packages/pwa-kit-dev/package-lock.json
npx npm@9 install

# Run npm install w/ these changes - should print warnings and exit 2
git switch wjh/lockfiles
npm install # using npm 7 or 8

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)

@wjhsf wjhsf requested a review from a team as a code owner July 28, 2023 17:35
Comment on lines +7 to +8
import path from 'node:path'
import fs from 'node:fs'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We always grumble about the scripts being stuck using require(), so this I made this as a .mjs file so it can use imports.

Comment on lines +21 to +22
.readdirSync(packagesDir, {withFileTypes: true})
.filter((dirent) => dirent.isDirectory())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pretty much just to avoid .DS_Store if it happens to be there.

@wjhsf wjhsf changed the title @W-13797015@ Enforce lockfile version 2 @W-13846314@ Enforce lockfile version 2 Jul 28, 2023
const lockfile = JSON.parse(fs.readFileSync(filename, 'utf8'))
if (lockfile.lockfileVersion !== LOCKFILE_VERSION_TARGET) {
console.error(
`Expected ${lockfile.name} to have lockfile version ${LOCKFILE_VERSION_TARGET}, but saw version ${lockfile.lockfileVersion}.`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we locking in version 2? As far as I know, lockfile version 3 are compatible with lock file version 2, the only concern is when lock files is generated in version 1, which is from Node 14. When Node 14 is offically out of support, we should not have this issue i believe

Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming what @alexvuong said: https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json#lockfileversion

Why do we want to lock in v2? To ensure compatibility with Node 14 / npm 6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to lock to some version in order to ensure consistency across packages. Version 2 was an arbitrary choice because that's what we're already using.

Copy link
Contributor

@vcua-mobify vcua-mobify left a comment

Choose a reason for hiding this comment

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

Testing this change - it works

Expected pwa-kit to have lockfile version 2, but saw version 3.
To regenerate the lockfiles with the correct lockfile version, please run `rm -rf package-lock.json node_modules/` in each impacted directory, and then use npm 7 or 8 to run `npm install` in the monorepo root.
npm ERR! code 1
npm ERR! path /Users/vcua/Git/pwa-kit
npm ERR! command failed
npm ERR! command sh -c -- node ./scripts/bootstrap.js && node ./scripts/check-lockfiles.mjs

@alexvuong
Copy link
Collaborator

Is this a temporary script until Node 14 is deprecated?

@wjhsf
Copy link
Contributor Author

wjhsf commented Aug 3, 2023

Is this a temporary script until Node 14 is deprecated?

I intended for it to be permanent; if we want it to be temporary we'll have to wait at least until we drop support for npm 8 (and hope that npm 10+ use lockfile v3), as npm 8 uses lockfile v2, but npm 9 uses lockfile v3.

@wjhsf wjhsf merged commit 1464c07 into develop Aug 3, 2023
20 checks passed
@wjhsf wjhsf deleted the wjh/lockfiles branch August 3, 2023 18:32
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.

None yet

4 participants