Repository navigation
Conversation
zzet
left a comment
zzet
left a comment
There was a problem hiding this comment.
Thanks. The import-linking half is careful work:
exportsis read with Node's rules: exact, conditional, wildcard, arrays andnullblocks.- Scoped names and npm/pnpm workspace members are handled.
- The importer's own package,
npm:aliases and workspace escapes stay external. - The per-binding import binds a symbol only when the extractor marks it public.
The tests cover both backends, and CI is green.
I'm requesting changes because linking the import also produces wrong cross-repo call edges.
Before merge
1. A linked import makes the whole provider repo match bare calls by name.
Call resolution doesn't use the per-binding import edge this PR writes. buildReachableReposIndex (internal/resolver/cross_repo.go:784-822) only records that the consumer file imports something from the provider repo. resolveFunctionCall (cross_repo.go:1309-1326) then takes the first function with that name anywhere in that repo. resolveTrackedNpmImports runs before that index is built (cross_repo.go:386), so every file that imports a tracked package gets this. Take a provider that exports f from src/index.ts and also has unexported f helpers in src/a/helpers.ts and src/b/helpers.ts:
- the import binds
provider/src/index.ts::f, as intended; - the call binds
provider/src/b/helpers.ts::fon the in-memory graph (5 of 5 runs) andprovider/src/a/helpers.ts::fon SQLite.
The name doesn't even have to come from the provider. Take import { debounce } from 'lodash'; debounce() in a file that also imports from the provider. The call binds the provider's unexported src/internal/util.ts::debounce; without the provider import it stays unresolved::debounce. On main all of these calls stay unresolved. In a code graph a wrong edge is worse than a missing one, and JS/TS files import common names from many packages, so this is likely to be common.
The tests use a name that is unique in the provider, so they don't catch it. Please resolve a JS/TS bare call through the caller file's own import bindings, meaning an import edge whose target ends in ::<name>. Also don't count npm-linked repos as reachable for plain name matching. Tests with a duplicate unexported name and an unrelated-package import would pin it.
2. Ambiguity is decided after probing, so a fork can win.
The description says the import stays external when several tracked repositories provide the package. But local and foreign (internal/resolver/npm_package.go:53-74) only collect providers whose entry file was found (if node == nil { continue }), and the check at :81 counts only those.
Say a real package has main: ./dist/index.js, which isn't indexed (see 5). A fork or example app with exports: ./src/index.ts sits in the same workspace. The consumer links to the fork. If both entries probe, the import correctly stays external::p. Please count the manifests that claim the name in each tier before probing, as the caller's own-repo check at :46-52 already does.
Worth fixing here
3. Provider edits and untracks drop the consumer's imports.
After linking, the consumer's import edges point at provider nodes. A save of the provider's entry file, or an untrack, evicts those nodes. Incoming import edges are then deleted rather than restubbed: on a save, restubIncomingRefsFromView and restubIncomingRefs restub only IsResolvableRefEdge kinds, which leave out EdgeImports (internal/indexer/incremental_batch.go:971, internal/indexer/indexer.go:5254). The consumer imports nothing from the package until it's edited or reindexed. After a save the call becomes unresolved::f with CrossRepo still true; after an untrack the call edge is gone too. On main the same edges stayed external::p and external::p::f.
The mechanism isn't new: relative TS imports lose their edges the same way. But this PR puts every npm consumer of a tracked package on it, in exactly the setup where both repos are being edited. Keep the original specifier on the edge (for example in Meta) and restub to external::<spec> on eviction. The next pass can then relink.
4. Links are never re-checked.
trackedNpmImportSpec (npm_package.go:152-163) only retries external:: and unresolved::import:: targets. Once an edge points at a provider node, it stays linked even after RunPreEnrichResolve and RunGlobalResolve, in any of these cases:
- the provider renames its package;
- the provider changes
exportsto block the path; - the consumer removes the dependency or switches to
npm:.
With the marker from 3, the whole-graph pass could re-check linked edges and revert those the lookup no longer selects.
5. Packages whose entry points name build output (dist) won't link, and that isn't documented.
Only name, exports, types and main are read (internal/indexer/npm_tracked_package.go:58-63). pickConditionalTarget reads only import and default, and dist/ is a built-in exclude. So these shapes never link, even with src/index.ts present:
mainandtypesboth pointing atdist;exports: {types: ./src/index.ts, import: ./dist/index.js}.
That is conservative and creates no wrong edges, but it's the usual shape. Please list it under Limits, and consider the types condition when the selected target isn't indexed.
Small things
6. file:, link: and portal: dependencies on a sibling tracked repo never link. npmSpecResolvesInRepo (internal/indexer/npm_alias_resolve.go:279-290) treats them as in-repo even when the path leaves the repository, though in local multi-repo work they're the most direct evidence there is. A follow-up is fine, plus a line under Limits.
7. Nothing tests that a provider in the caller's own workspace wins over an eligible foreign one. Replacing the two tiers at npm_package.go:75-78 with one merged list still passes every test this PR adds.
8. Take a subpath whose exports entry has only unsupported conditions. The fallback then probes <pkg>/utils(.ts|/index.ts) (npm_tracked_package.go:112-117), a path the manifest doesn't name, and could bind an unrelated top-level utils/ folder.
9. docs/multi-repo.md ("Cross-repo edges") doesn't mention package-name linking or its limits.
After #806
#806 is planned to land first. The rebase has two small conflicts, in ResolveAllContext and in the incremental frontier. The npm retry belongs in the body of #806's ResolveMutationFrontiersBounded, not in the ResolveMutationFrontiers wrapper. No ResolutionSemanticsVersion bump is needed, because this only changes the cross-repo pass and no generation payload includes it.
|
Hi @peterkc, thank you for this PR! #806 (incremental index write amplification) was merged into
When you have a moment, could you please:
Thanks again, and let us know if anything in the new code is unclear! |
A JS/TS import of a package the importer declares in package.json stayed external, even when the package's source repository was tracked: the declared-dependency skip ended the directory cascade, and the package's entry file has no package-qualified name. The cross-repo pass now retries such imports before call reachability. It maps the package name to a tracked repository through package.json names and npm or pnpm workspace members, and picks the entry through exports, types, main or the root index. A symbol binds only when the TypeScript extractor marks it public; otherwise the import binds to the file. The import stays external when the importer's own repository provides the package, when several tracked repositories in the workspace provide it, for npm: aliases, and for workspace paths that leave the repository. The same-repository directory skip from c821417 is unchanged. Manifests are read lazily, once per resolver, only when a declared import reaches the lookup. Missing-target probes are memoized per pass. Known limits: a JavaScript package binds to the file, since only the TypeScript extractor records visibility; provider-side changes (tracking the provider later, editing its package.json or entry file) reach consumer imports only at the next whole-graph pass or a consumer edit.
When package.json declares exports, Node resolves the package only through it. The tracked-package lookup still fell back to types, main, the root index or a subpath folder, so it could bind an import that Node refuses: a root the exports map omits, a subpath it does not map, or one it blocks with null. The lookup now records which entries exports declares, independently of the conditions parsePackageExports supports. A declared entry with a supported target binds that target. A declared entry with only other conditions (types, require, node) falls back to the legacy candidates, as before. An entry exports does not declare, or declares as null, stays external. A null exports field counts as absent, as in Node. ResolveAllContext now checks its context while it scans import edges for tracked packages and between hydration pages, and returns the context error.
…gets A tracked package whose exports field is an array had no declared entries, so every import of it stayed external. Node reads such an array as an ordered list of root targets and takes the first supported one before it looks at the filesystem. The lookup now declares the root for an array and picks its first item that is a "./" path or a conditional object with a condition the indexer reads, searching nested arrays in order and skipping null and invalid items. That target is authoritative, as for other exports entries. An array whose items are all null, or an empty array, blocks the root. An array with only unsupported conditions keeps the legacy candidates. Subpaths stay unexported. The NpmPackageLookup comment now states that legacy candidates also apply to a declared entry without a supported target.
…tion
An exports array item such as {"import": null} names a condition the
indexer reads, but its target is null or invalid. Node resolves such an
item to nothing, so the root stays unexported. The lookup treated it
like an object with only unknown conditions and fell back to main or
the root index.
Only objects without a recognized condition now leave the root to the
legacy candidates.
… links on provider changes Review follow-up. A linked import no longer makes the whole provider repo match bare calls by name: JS/TS calls resolve through the caller file's own per-binding imports (renamed imports by their local alias, recorded as import_alias in edge Meta), and npm-linked repos no longer count as reachable for plain name matching. Default-import calls stay unresolved. Ambiguity counts the manifests that claim a package in each tier before probing, so a fork cannot win when the real package's entry is not indexed. Linked imports keep their specifier; provider saves, full reindexes and untracks park them as external::<spec> with their calls, and the whole-graph pass re-checks linked edges and reverts those the lookup no longer selects, removing stale cross_repo_calls edges. Also: the types condition is tried when the selected target is not indexed, no fallback probe for exports entries with only unsupported conditions, a test that an own-workspace provider wins, and docs.
|
Moving this back to draft while I rebase onto |
…track After the rebase onto zzet#806, two review findings remained: - A full provider reindex or untrack/retrack parked consumer imports to external:: and their calls to unresolved::, but IndexRepo and track had no cross-repo pass for those consumers, so the links stayed lost until a whole-graph resolve. The lifecycle test hid this by calling RunPreEnrichResolve. Parking now returns the exact consumer files; a save carries them into the batch's cross-repo frontier, and IndexRepo and track run the scoped cross-repo resolver on consumers whose package the provider now claims. The test no longer forces a whole-graph resolve. - The binding map used an import's alias only for npm imports, so a resolved non-npm `{ f as g }` import exposed `f`. The map is now built only from hydrated cross-repo JS/TS imports and always uses the local binding name.
…rder Review of the rebase onto zzet#806 found four more cases: - On SQLite a full shadow replacement of the provider (warm restart with full_retrack) went through EvictRepoForShadowReplacement, which skipped the npm parking, so the consumer's imports and calls were deleted; main keeps them as external stubs. Parking now runs before that call too, and a full retrack relinks parked consumers when resolution is not deferred. - A warm restart after an offline provider edit parked the consumer, but the exact warm resolve saw only the provider's derived files. IndexResult now carries the parked consumer files in a separate CrossRepoResolveFiles, which the daemon's warm frontier adds to the resolve set; derived plans stay provider-only. - Tracking a consumer after its provider left it unlinked until a whole-graph pass, because only imports claimed by the newly tracked repo were retried. The repo's own external imports claimed by any tracked package are now retried too. - A root exports object whose recognized condition is null (for example {"import": null}) still fell back to types, main or index. It now blocks the root, as the array form already did. The docs now say that an import of a missing or non-public symbol links only to the entry file, and list two existing limits: lenient non-"./" export targets, and file-scoped call binding.
780a691 to
bc59652
Compare
|
Rebased onto main after #806 and addressed the review:
The npm retry sits in |
|
The one CI failure (macos-latest, indexer-0: TestCheckoutReadyWarmupYieldsToTheCheckoutsForegroundWork, 'a checkout with an admitted ticket is touched') is a timing-sensitive warm-up test in code this PR does not touch; it passes 5/5 locally at bc59652. A rerun should clear it. |
Summary
A JS/TS import of an npm package that the importer declares in
package.jsonstayedexternal::even when the package's source repository was tracked. The declared-dependency skip ends the directory cascade, and the package entry file has no package-qualified name. The cross-repo pass now links these imports, and calls through the file's own import bindings, to the tracked repository.Diagrams
Changes
package.jsonnames and npm or pnpm workspace members, then pick the entry throughexports(Node's rules, including arrays andnullblocks),types,mainor the root index.external::<spec>instead of deleted, and the next cross-repo pass relinks them.exportsblocks the path, dependency removed).ResolveMutationFrontiersBounded. A save carries its parked consumer files into the batch's cross-repo frontier; a full index, track or warm restart retries the consumers whose package the provider claims.exportstypescondition is tried. A subpath with only unsupported conditions no longer guesses a<package>/<subpath>folder.docs/multi-repo.md("Cross-repo edges") describes package-name linking, the ambiguity rules, lifecycle behavior and the limits below.Limits
dist/) stay external unless an indexed target is declared.file:,link:andportal:dependencies on a sibling tracked repository are not linked yet; follow-up planned../export targets are read leniently, as on main. Call binding is file-scoped, so a same-named parameter or local can bind to the import.Testing
All tests pass (
go test -race ./...)New tests added for new functionality
Benchmarks run if performance-relevant
Two-repo fixtures fail on main and pass here, on the in-memory graph and SQLite, including a duplicate unexported name in the provider and an unrelated-package import (point 1).
Lifecycle tests cover provider save, full reindex, warm-restart full retrack, an offline provider edit through the daemon warm frontier, untrack and retrack, in both track orders, without forcing a whole-graph resolve.
With main's versions of the changed functions swapped in, 15 of the new or changed tests fail and 10 pass; four existing TS and Go tests give the same result on both.
The edit-delta and parity group, build and
go vetpass locally. Race and lint run in CI.Checklist
Meta["methods"]for interfaces (if applicable)EdgeMemberOfedges to their containing type (if applicable)