-
-
Notifications
You must be signed in to change notification settings - Fork 937
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
8.7.0 regression: Install of monorepo that contains locally injected packages fails #7040
Comments
Add (currently failing) test for injected installs. Represents regression issue discussed here: pnpm#7040
Ensure that fetches from directory are delayed until the package is synlinked. That will allow the created `exportableManifest` to correctly find it's dependencies. Resolves: pnpm#7040
Ensure that fetches from directory are delayed until the package is synlinked. That will allow the created `exportableManifest` to correctly find it's dependencies. Resolves: pnpm#7040
Current understanding of how pnpm works internallyWhen performing an install, pnpm kicks off a bunch of asynchronous "fetchers". This can perform various IO like downloading tarballs. It also can load packages internally (directory fetcher). This work can be incredibly IO heavy, and is not immediately needed, but it kicked off early to optimize performance. NOTE this means that the fetchers can fail early in install, but not have the failure reported until later (once actually used) Later on in the install, the "fetch" result is actually referenced https://github.com/pnpm/pnpm/blob/main/pkg-manager/core/src/install/link.ts#L428 At an even later step, the local packages are sym-linked together: https://github.com/pnpm/pnpm/blob/main/pkg-manager/core/src/install/link.ts#L273 The regression occurs because the directory fetcher expects the dependencies to be sym-linked, which does not happen until after the fetch is complete. Which is a circular dependency, and fails. SolutionThere are two parts of this. First, we need to defer the execution of the directory fetcher. We don't want to delay all fetchers (e.g. tarballs) because that provides significant performance boost and do not have a similar dependency issue. Second, we need to perform the sym-linking before the fetching is called, that will ensure the local packages are "installed" before being referenced. |
Opened up PR adding a test to demonstrate this regression: https://github.com/pnpm/pnpm/pull/7048/files Note question regarding expected format of If we expect this package to be "exportable" (e.g. If we do not expect the fetched directory to be "exportable", then the solution can perhaps be simpler and simply move the logic to a deploy-specific method |
Opened a separate PR as a possible implementation of the solution. There are couple parts that feel a bit hacky... trying to delay invocations of promises and immediately invoking in others... Definitely open to alternate solutions or implementationos |
A reported failure in keycloak is further impacted by this issue. Even once the dependencies are resolved, it reports not-installed. That fix is trivial enough on their side, but probably could benefit from better messaging. Can consider as improvement to DX once solution is implemented |
Ensure that fetches from directory are delayed until the package is synlinked. That will allow the created `exportableManifest` to correctly find it's dependencies. Resolves: pnpm#7040
This issue was fixed in this PR #7058 nacho@Ignacios-MacBook-Pro pnpm-install-error-example % pnpm i
Scope: all 4 workspace projects
Packages: +3
+++
Progress: resolved 3, reused 2, downloaded 1, added 3, done
package-b prepare$ pnpm run build
│ > @namespace/package-b@1.0.0 build /private/tmp/pnpm-install-error-example/package-b
│ > tsc ./src/index.ts --outDir ./dist
└─ Done in 1s
Done in 1.5s
nacho@Ignacios-MacBook-Pro pnpm-install-error-example % pnpm -v
8.7.4 |
Add (currently failing) test for injected installs. Represents regression issue discussed here: pnpm#7040
Add (currently failing) test for injected installs. Represents regression issue discussed here: pnpm#7040
Add (currently failing) test for injected installs. Represents regression issue discussed here: pnpm#7040
Last pnpm version that worked
8.6.x
pnpm version
8.7.0
Code to reproduce the issue
Run
pnpm i
in this repo https://github.com/geekytime/pnpm-install-error-exampleExpected behavior
Install is successful
Actual behavior
Fails on error
Additional information
Broken by this PR: #6943
Which attempted to apply
publishConfig
to the deploy step. However it impacts some installs which hit a circular dependency (expecting to be installed before install is complete)See additional conversation in this issue: #6994
Creating a new issue to track different conditions (original issue related to actual
deploy
, and did not haveinstall
run prior`)Will attach further context in comments
Node.js version
20.5.1
Operating System
macOS
The text was updated successfully, but these errors were encountered: