Skip to content

fix(dependency-path): fix lockfile v6 version parsing for a patched dependency#5964

Closed
valeneiko wants to merge 1 commit intopnpm:mainfrom
valeneiko:fix-patched-dep-parsing
Closed

fix(dependency-path): fix lockfile v6 version parsing for a patched dependency#5964
valeneiko wants to merge 1 commit intopnpm:mainfrom
valeneiko:fix-patched-dep-parsing

Conversation

@valeneiko
Copy link

Previously pnpm would fail to resolve patched package when lockfile v6 is used. This change correctly handles patch hash in the version suffix.

@valeneiko valeneiko requested a review from zkochan as a code owner January 21, 2023 00:25
@welcome
Copy link

welcome bot commented Jan 21, 2023

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

version: '1.0.0',
})

expect(parse('/foo/1.0.0_k5brw22k7hadiw3hedogf4eiee(bar@1.0.0)')).toStrictEqual({
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we need to make this syntax a bit more readable in the new format.

We could change it to something like

Suggested change
expect(parse('/foo/1.0.0_k5brw22k7hadiw3hedogf4eiee(bar@1.0.0)')).toStrictEqual({
expect(parse('/foo/1.0.0(patch_hash=k5brw22k7hadiw3hedogf4eiee)(bar@1.0.0)')).toStrictEqual({

Copy link
Author

Choose a reason for hiding this comment

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

Not sure. It makes sense for patch hash to be part of the version. It might blend in with peer dependencies too much otherwise.

But up to you, we can close this PR if you plan on updating the format.

Copy link
Member

Choose a reason for hiding this comment

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

I think with my suggestion it is easier to parse the dependency path because the version will always end with (. With your suggestion the version will end with either _ or (.

Copy link
Author

Choose a reason for hiding this comment

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

The input to the parse function has _ in it. If I change the function to return (patch_hash=....) then I am getting:

Broken lockfile: no entry for '/ts-node/10.9.1_k5brw22k7hadiw3hedogf4eiee(@swc/core@1.3.27)(@types/node@18.11.18)(typescript@4.9.4)' in pnpm-lock.yaml

Which makes sense since in the packages list we now have this, but all the references to ts-node have the path from the error.

/ts-node@10.9.1(patch_hash=k5brw22k7hadiw3hedogf4eiee)(@swc/core@1.3.27)(@types/node@18.11.18)(typescript@4.9.4)

I am going to close my PR, since it does not work either.
If I delete node_modules and run pnpm i with the updated lock file, I am getting the following error:

Package name mismatch found while reading {"integrity":"sha512-NtVysVPkxxrwFGUUxGYhfux8k78pQB3JqYBXlLRZgdGUqTO5wU/UyHop5p70iEbGhB7q5KmiZiU0Y3KlJrScEw==","registry":"https://registry.npmjs.org/","tarball":"https://registry.npmjs.org/ts-node/-/ts-node-10.9.1_k5brw22k7hadiw3hedogf4eiee.tgz"} from the store. This means that the lockfile is broken. Expected package: ts-node@10.9.1_k5brw22k7hadiw3hedogf4eiee. Actual package in the store by the given integrity: ts-node@10.9.1

…ependency

Previously pnpm would fail to resolve patched package when lockfile v6 is used. This change correctly handles patch hash in the version suffix.
@valeneiko
Copy link
Author

Closing, since solution in PR does not work

@valeneiko valeneiko closed this Jan 21, 2023
@valeneiko valeneiko deleted the fix-patched-dep-parsing branch January 21, 2023 16:12
@zkochan
Copy link
Member

zkochan commented Jan 22, 2023

Filed an issue about it: #5967

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.

2 participants