fix(install): delay fetching directory until symlink #7049
Conversation
| @@ -134,6 +135,53 @@ export async function linkPackages ( | |||
| failOnMissingDependencies: true, | |||
| skipped: new Set(), | |||
| }) | |||
| let linkedToRoot = 0 | |||
There was a problem hiding this comment.
This section is just moving the sym link logic up before the linkNewPackages (which invokes the fetcher internally)
It is otherwise copy-pasted.
Now the directory fetcher will have locally linked packages setup as necsessary
| ) | ||
| linkedToRoot = await linkDirectDeps(projectsToLink, { dedupe: opts.dedupeDirectDeps }) | ||
| } | ||
| await opts.finishLockfileUpdates() |
There was a problem hiding this comment.
finishLockfileUpdates is required to be called because it resolves the requiresBuild promise https://github.com/pnpm/pnpm/blob/main/pkg-manager/resolve-dependencies/src/index.ts#L349, which is used inside linkNewPackages
It can't be invoked before sym linking, because it triggers the fetch internally
| let blocker: Promise<void> | ||
|
|
||
| if (opts.pkg.resolution.type === 'directory') { | ||
| const blocked: pDefer.DeferredPromise<void> = pDefer() |
There was a problem hiding this comment.
I'm not in love with this implementation, but basically we want to block the execution of doFetchToStore until actually invoked
only apply this to directory fetchers though, others like tarball should still get kicked off.
Alternative approach is instead of passing the blocker promise, invoke doFetchToStore(filesIndexFile, fetching, target, blocker) in the promiseSharer callback... Would need to be careful about deduping
| return { | ||
| dependenciesByProjectId, | ||
| dependenciesGraph, | ||
| finishLockfileUpdates: promiseShare(finishLockfileUpdates(dependenciesGraph, pendingRequiresBuilds, newLockfile)), | ||
| finishLockfileUpdates: () => { | ||
| finishLockfileUpdatesPromise ??= finishLockfileUpdates(dependenciesGraph, pendingRequiresBuilds, newLockfile) |
There was a problem hiding this comment.
finishLockfileUpdates calls fetching internally, so we should defer this logic until necessary https://github.com/pnpm/pnpm/blob/main/pkg-manager/resolve-dependencies/src/index.ts#L337
Another alternative approach is to have it depend on some "lazy" version of fetching that does not trigger the invocation, but will subscribe to the underlying promise...
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
JacobLey
force-pushed
the
fetchDirAfterLink
branch
from
8828730
to
38d4c84
Compare
| @@ -52,3 +53,124 @@ test('install with no store integrity validation', async () => { | |||
|
|
|||
| expect(fs.readFileSync('node_modules/is-positive/readme.md', 'utf8')).toBe('modified') | |||
| }) | |||
|
|
|||
| describe('install injected dependencies', () => { | |||
There was a problem hiding this comment.
Woops didn't mean to include with this PR (#7048) but hopefully serves as proof of passing tests...
|
I would rather look into other solutions as this will probably make installation speed worse. |
Depending on desired install behavior (i.e. should injected dependencies have an exportable manifest) (see #7048 (comment)) we can optionally pull all this new logic out the critical install path And only apply it to deploy (as that is where feature originally was supposed to only impact). Even if we want injected packages to be exportable... perhaps we could perform that retroactively after all the sym linking (and leave the directory fetcher logic alone) |
|
Note with revert of original deploy PR, this is not really in a mergeable state. But would like to keep it open for point of discussion about how to best implement deploy functionality, while either:
|
|
What if we just upgrade |
Ensure that fetches from directory are delayed until the package is synlinked. That will allow the created
exportableManifestto correctly find it's dependencies.Resolves: #7040