fix: pass storeDir to createClient to fix resolution skipping#10502
fix: pass storeDir to createClient to fix resolution skipping#10502
Conversation
|
Ready for review? I actually wanted to make storeDir required. That would have caught the issue. But it required too many changes |
|
I'll mark as ready for review! Was deciding whether it was worth adding a changelog and tests. I think the answer to both is no since the original commit hasn't been released yet and tests would need to be E2E. I see unit tests didn't catch this originally since |
There was a problem hiding this comment.
Pull request overview
This PR fixes a performance regression in pnpm install by ensuring the storeDir option is properly passed to the client creation function. This prevents resolution skipping from being bypassed due to an uninitialized peekManifestFromStore function.
Changes:
- Added
storeDirparameter to the client configuration increateNewStoreController
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Yeah, that makes sense on both fronts. |
|
We should add some performance regression tests but we need a big project like the one you are using. Looks like the project I use is not massive enough. |
|
Agree — I feel bad that it's not possible for you to test pnpm against this proprietary repo. I can look into creating a public copy of it without source code, but similar |
|
There is one open-source package that is very big: @teambit/bit |
Prior discussion: #10409 (comment)
When running
pnpm installwith minor changes to apackage.jsonfile, I'm seeing a performance regression duestoreDirbeingundefinedbelow. This causespeekManifestFromStoreto never be initialized.pnpm/resolving/npm-resolver/src/index.ts
Lines 187 to 188 in 29a3151
I tested this PR and passing through
storeDirhere fixes the performance issue!