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 JavaScript workspace lockfile generation #1237

Merged
merged 8 commits into from
Oct 2, 2023
Merged

Fix JavaScript workspace lockfile generation #1237

merged 8 commits into from
Oct 2, 2023

Conversation

cd-work
Copy link
Contributor

@cd-work cd-work commented Sep 19, 2023

This patch fixes the lockfile generation for workspaces with npm, yarn, and pnpm.

See #1236.

This patch fixes the lockfile generation for workspaces with npm, yarn,
and pnpm.

See #1236.
@cd-work cd-work requested a review from a team as a code owner September 19, 2023 18:49
@cd-work cd-work requested a review from maxrake September 19, 2023 18:49
lockfile_generator/src/pnpm.rs Outdated Show resolved Hide resolved
lockfile_generator/src/yarn.rs Outdated Show resolved Hide resolved
lockfile_generator/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@maxrake maxrake left a comment

Choose a reason for hiding this comment

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

The changes were tested locally and found to work for npm (using a self-created workspace setup) and pnpm (using a checkout of SvelteKit).

For yarn, a checkout of Facebook's react repo was used as an example of a modern yarn v3.x workspace project. The changes in this PR failed to parse the package.json file from within the packages/react-art directory (which was just selected as a quick test).

../react/packages/react-art  2  6 on  main is 📦 v18.2.0 via  v20.6.1
❯ /Users/maxrake/dev/phylum/localdev/cli/target/debug/phylum parse -t yarn ./package.json > parse.json
Generating lockfile for manifest "package.json" using Yarn…
❗ Error: could not parse lockfile: ./package.json

Caused by:
    0: Lockfile generation failed! For details, see: https://docs.phylum.io/docs/lockfile_generation
    1: package manager exited with error code 1:

lockfile_generator/src/yarn.rs Outdated Show resolved Hide resolved
@cd-work cd-work requested a review from maxrake September 27, 2023 19:09
maxrake
maxrake previously approved these changes Sep 29, 2023
Copy link
Contributor

@maxrake maxrake left a comment

Choose a reason for hiding this comment

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

The code was reviewed and the changes tested locally. This time, for yarn, the jest project was used as a true representative example of a modern Yarn project. The changes worked as expected, for both the project root and for individual workspaces.

LGTM.

kylewillmon
kylewillmon previously approved these changes Oct 2, 2023
lockfile_generator/src/npm.rs Outdated Show resolved Hide resolved
@cd-work cd-work dismissed stale reviews from kylewillmon and maxrake via 48849ea October 2, 2023 20:08
@cd-work cd-work requested a review from kylewillmon October 2, 2023 20:08
@cd-work cd-work enabled auto-merge (squash) October 2, 2023 20:12
@cd-work cd-work merged commit 50388ec into main Oct 2, 2023
13 checks passed
@cd-work cd-work deleted the js_workspaces branch October 2, 2023 20:17
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.

3 participants