Skip to content

Check in lockfile and use npm consistently #1340

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chaance
Copy link
Contributor

@chaance chaance commented Aug 15, 2025

Description

We should be checking in the lockfile to ensure dependencies are consistent across environments. Since we use npm in CI it seems to make sense that we use it on the development side as well.

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

@chaance chaance requested a review from a team as a code owner August 15, 2025 16:39
@chaance chaance requested review from dandorman and gcarvelli August 15, 2025 16:39
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR standardizes the project's package management by migrating from Yarn to npm across all environments. The changes include:

  • Package management migration: Updates package.json to use npm run build instead of yarn run build in the prepublishOnly script
  • Lockfile strategy change: Removes package-lock.json from .gitignore and adds yarn.lock to it, ensuring npm's lockfile is tracked in version control for consistent dependency resolution
  • Build script updates: Modifies bin/restore-or-install to use npm commands (npm install and npm ci) instead of yarn, while maintaining the same caching logic based on package-lock.json checksum
  • Documentation alignment: Updates installation instructions in README.md from yarn add @workos-inc/node to npm i @workos-inc/node

The migration addresses environment consistency since the CI pipeline already uses npm. By checking in the lockfile, the project ensures deterministic dependency resolution across development, CI, and production environments. This eliminates potential "works on my machine" issues caused by different package managers resolving dependencies differently. The changes maintain existing functionality while standardizing the toolchain.

Confidence score: 4/5

  • This PR is generally safe to merge with proper coordination across the development team
  • Score reflects solid technical changes but requires team coordination to ensure all developers switch from yarn to npm
  • Pay close attention to the bin/restore-or-install script logic with npm ci usage

4 files reviewed, 1 comment

Edit Code Review Bot Settings | Greptile

cache store $KEY ./node_modules
fi

if [[ ! `ls -d ./node_modules/.bin 2>/dev/null` ]]; then
yarn install --check-files
npm ci
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Using npm ci here may cause issues since it removes existing node_modules before installing. If the first condition already installed dependencies, this will unnecessarily reinstall everything. Consider using npm install or checking if this condition is actually needed.

Suggested change
npm ci
npm install

@chaance chaance requested a review from nicknisi August 15, 2025 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants