Conversation
codykaup
added
release
|
📦 Package Size: 7180 KB |
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1502 +/- ##
==========================================
+ Coverage 86.88% 86.91% +0.03%
==========================================
Files 274 274
Lines 5499 5512 +13
Branches 1488 1497 +9
==========================================
+ Hits 4778 4791 +13
Misses 635 635
Partials 86 86 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ea897b3 to
40d04e5
Compare
codykaup
changed the title
66a2ff4 to
4ed58d2
Compare
justin-thurman
left a comment
justin-thurman
left a comment
There was a problem hiding this comment.
Overall, looks good. I think the package version issue is blocking, but should be an easy fix. Also worth calling out that I think this won't work for workspaces in a subdirectory, since, unless I'm misreading, findChangedDependencies checks each package's own directory for a lockfile, and then the repo root, but not subdirs. And maybe the same gap exists for yarn already? Seems like this would have come up before though, so maybe I'm missing something. In any case, not a regression, so debatable whether it should be added in this PR or not.
Side note:
We added pnpm-lock.yaml as a SUPPORTED_LOCK_FILES but didn't add it to the isPackageLockFile check, so a lockfile-only change never entered the TurboSnap code path.
Any way to enforce that SUPPORTED_LOCK_FILES and isPackageLockFile can't drift? If not something structural, then a unit test that parameterizes SUPPORTED_LOCK_FILES and just ensure each one is supported by isPackageLockFile?
4ed58d2 to
9e6973a
Compare
checkoutFile now writes to tmpdir/<fileName> instead of tmpdir/<basename>, so files checked out together keep the layout they have in the repository.
getDependencies now computes the importer from the manifest and lockfile paths it already receives, so callers no longer pass it. findChangedDependencies stops flattening HEAD files into a temp directory and checks baseline files out to their repository-relative paths. The flatten-and-copy workaround for snyk-nodejs-plugin's inspect moves next to the inspect call.
9e6973a to
e6af1f6
Compare
The single-project parser still resolves against the root importer when the lockfile has one, so only deps the root lacks keep raw specifiers. The workspace-link test name now says what it asserts: catalog entries resolve, and workspace links come out the same on HEAD and baseline.
Assert the pnpm importers by name rather than by argument position, remove the temp directory the unparseable-lockfile test creates, and pass repository-relative paths in the historic-files test to match the getDependencies contract.
The checked-out file now lives at its repository-relative path under tmpdir, so a directory name with a space or shell metacharacter would break the redirect.
findChangedDependencies no longer uses the path checkoutFile returns, so the mock's fake result and the comment describing it were misleading.
Record the importer each workspace-parser call receives and compare the full multiset, so each importer is proven to be used for both HEAD and the baseline. Note that the changed-dependency result comes from the mocked graph, so the importer assertion is what exercises the pnpm path.
Callers know where the file lands because it keeps its repository-relative path under tmpdir. Returning the path suggested a second contract.
|
@justin-thurman quite a few changes because I pulled out the |
Correct but that still applies to all package managers. Something we can address in a separate PR if we run into it in the wild later! |
codykaup
added
skip-release
|
🚀 PR was released in |
chromatic-ci-bot
added
the
released
3 participants
Problem
TurboSnap missed dependency changes in PNPM projects in two layers:
pnpm-lock.yamlas aSUPPORTED_LOCK_FILESbut didn't add it to theisPackageLockFilecheck, so a lockfile-only change never entered the TurboSnap code path.package.jsonandpnpm-lock.yamlpair as if it wasn't a monorepo. Therefore, a dependency with the specifiercatalog:would produce<dependency>@catalog:instead of the real version number.Solution
utilities.tsfor a single source of truth.getDependenciesbranches on lockfile type. For pnpm it derives the manifest's importer (its directory relative to the lockfile) from the paths it is given, then drivesparsePnpmWorkspaceProjectdirectly socatalog:andworkspace:specifiers resolve against the right importer table. Baselines are checked out to their repository-relative paths under a temp directory so the manifest keeps its position relative to the lockfile.snyk-nodejs-lockfile-parseris pinned to exactly2.7.0to matchsnyk-nodejs-plugin's own pin, so only one copy is installed.package.jsonoutside the workspace globs) fall back toparsePnpmProject. That keeps raw specifiers instead of throwing, which is what the yarn and npm parsers already do for manifests the lockfile can't place.pnpm-workspace.yamlis deliberately not tracked. A catalog change there always co-changespnpm-lock.yaml, and tracking it would force full rebuilds for unrelated workspace config edits.📦 Published PR as canary version:
18.10.1--canary.1502.36458228651.0✨ Test out this PR locally via:
npm install chromatic@18.10.1--canary.1502.36458228651.0 # or yarn add chromatic@18.10.1--canary.1502.36458228651.0