fix(yarn-plugin-external-workspaces): support purely-internal externals#4117
Draft
Saadnajmi wants to merge 1 commit intomicrosoft:mainfrom
Draft
fix(yarn-plugin-external-workspaces): support purely-internal externals#4117Saadnajmi wants to merge 1 commit intomicrosoft:mainfrom
Saadnajmi wants to merge 1 commit intomicrosoft:mainfrom
Conversation
Two issues prevented the plugin from working with externals that exist
locally but were never published to npm:
1. getFinderFromJsonConfig read 'generated.generated.{...}' from the
config JSON, but the README + WorkspaceOutputJson type document the
shape as 'generated.{...}'. The double-nested access caused repoPath
and workspaces to always be empty, so findPackage always returned null
and lookups silently fell through.
2. The 'external: -> fallback: -> npm:' resolver chain hard-fails at the
npm step for packages that don't exist on the registry. The chain
exists to support semver coalescing against npm when externals also
live remote, but for purely-internal monorepo packages there is no
remote version to coalesce against. Add a fast path: when isLocal is
true and the local resolver is called, skip the chain and return a
direct external: locator. The fetcher already resolves locally via
workspace.localPath. getResolutionDependencies returns {} in the same
case so yarn doesn't pre-resolve a chain that won't terminate.
Validated against an Office monorepo workspace consuming three internal
externals from a sibling monorepo on disk; yarn install completes
cleanly, externals symlink to the sibling repo's package directories,
and dist/index.js resolves through the symlinks.
Contributor
Author
|
Normally Claude adds itself a co-author, but it didn't here. Regardless, marking as draft till I test more. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two issues prevented
@rnx-kit/yarn-plugin-external-workspaces(v0.1.3) from working with externals that exist locally but were never published to npm:1.
getFinderFromJsonConfigreads from a doubly-nestedgenerated.generatedconfiguration.ts:93-96reads:The first line strips one
generatedlayer, then the second line strips another. The README (and theWorkspaceOutputJsontype intypes.ts) both document the JSON shape as a single layer:{ "generated": { "version": "1", "repoPath": "...", "workspaces": {...} } }With that documented shape,
parsedJson.generatedisundefined, sogeneratedbecomes{}, andfindPackagealways returnsnull. Externals silently fall through to npm.Drop the extra
?.generatedso the documented shape works as advertised.2. Resolver chain hard-fails for packages not published to npm
The
external: → fallback: → npm:chain (workspace.ts) is wired such that yarn must resolve the npm step before resolution succeeds. The README says:— but the design also runs the npm step when the local files are present (for version coalescing). For purely-internal monorepo packages that simply don't exist on npm, the chain throws 404 even though we have a perfectly good local source.
Add a purely-local fast path: when
isLocal === trueandresolverType === "local",getCandidatesshort-circuits to a directexternal:locator (the fetcher already resolves it viaworkspace.localPath).getResolutionDependenciesreturns{}in the same case so yarn doesn't pre-resolve a non-existent chain.Test plan
Validated against a real Office monorepo workspace consuming three internal externals from a sibling monorepo on disk:
yarn installcompletes cleanly (2066 packages installed in ~38s, no 404s)node_modules/<internal-package> is a symlink to../..//`dist/index.js(sharedjs's pre-built artifacts) resolves through the symlinkWithout these fixes the install fails at:
Notes
npm-404 + local-presentregression test if you'd like.dist/bundle is gitignored, so the bundled output isn't part of this PR — you'll regenerate at release time.