-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(pnpm): correctly parse dependency paths with nested peer dependecies #8003
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
7 Ignored Deployments
|
🟢 Turbopack Benchmark CI successful 🟢Thanks |
@@ -170,9 +192,13 @@ mod tests { | |||
#[test_case("foo@1.0.0", DepPath::new("foo", "1.0.0") ; "basic v7")] | |||
#[test_case("@scope/foo@1.0.0", DepPath::new("@scope/foo", "1.0.0") ; "scope v7")] | |||
#[test_case("foo@1.0.0(bar@1.2.3)", DepPath::new("foo", "1.0.0").with_peer_suffix(Some("(bar@1.2.3)")) ; "peer v7")] | |||
#[test_case( | |||
"eslint-module-utils@2.8.0(@typescript-eslint/parser@6.12.0(eslint@8.57.0)(typescript@5.3.3))", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an abridged example the full christian name is eslint-module-utils@2.8.0(@typescript-eslint/parser@6.12.0(eslint@8.57.0)(typescript@5.3.3))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.6.1(@typescript-eslint/parser@6.12.0(eslint@8.57.0)(typescript@5.3.3))(eslint-plugin-import@2.29.0)(eslint@8.57.0))(eslint@8.57.0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💀
|
||
let (version, peer_suffix) = match version.find('(') { | ||
Some(paren_index) if version.ends_with(')') => { | ||
// TODO: safety of the split? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, old comment. I had an off by one error previously 😅
🟢 CI successful 🟢Thanks |
Description
Should address the reported issues in #7993
I missed a change to the dependency path format where nested peer dependencies are now shown e.g. if a has peer dependency b and b has peer dependency c, then the resulting dependency path will be
a@1.0.0(b@1.0.0(c@1.0.0))
.Not sure when dependency path parsing got simplified, but I believe I missed the host segment getting dropped which greatly simplifies the format. The new parsing logic is a faithful port of the current JS parsing code: https://github.com/pnpm/pnpm/blob/main/packages/dependency-path/src/index.ts#L91
Testing Instructions
Added unit test for parsing a dependency path that contains a nested peer dependency. Existing test suite.
Closes TURBO-2848