feat: support workspace subdirectory resolution with WithPathBase and WithPackageDeps#39
Merged
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR adds two new configuration options to the local resolver: ChangesPath Base and Package Dependencies Scoping
Sequence DiagramsequenceDiagram
actor Caller
participant CLI/WASM as CLI or WASM
participant Resolver as Local Resolver
participant FileSystem as FileSystem<br/>& PackageJSON
participant ImportMap as ImportMap
Caller->>CLI/WASM: Call with --path-base /examples/kitchen-sink --package-deps /path/to/package
CLI/WASM->>Resolver: Create resolver with WithPathBase() and WithPackageDeps()
activate Resolver
Resolver->>Resolver: Store pathBase and packageDeps
deactivate Resolver
Caller->>Resolver: Resolve(workspaceRoot)
activate Resolver
Resolver->>Resolver: normalizePathOptions() — resolve pathBase/packageDeps relative to root
Resolver->>FileSystem: Read packages from workspace
alt packageDeps is set
Resolver->>FileSystem: Read specific package.json at packageDeps path
FileSystem-->>Resolver: Dependencies from targeted package
Resolver->>Resolver: Gather only those dependencies
else packageDeps not set
Resolver->>FileSystem: Read all workspace packages
FileSystem-->>Resolver: All dependencies
Resolver->>Resolver: Gather all workspace dependencies
end
Resolver->>FileSystem: Resolve node_modules and graph relationships
FileSystem-->>Resolver: Computed imports and scopes
alt pathBase is set and differs from root
Resolver->>ImportMap: rebaseImportMapForPathBase() — rebase paths relative to pathBase
ImportMap-->>Resolver: Rebased import map
end
Resolver-->>Caller: Return result with rebased/filtered imports
deactivate Resolver
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Build Artifacts
Built from 455c9ae @ feat/relative-root |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
resolve/local/local.go (1)
578-596: 💤 Low valueDrop the unnecessary inner block.
The bare
{ ... }only scopesownerNamefor the lookup. Inlining it (or extracting a small helper for owner attribution) reads more naturally and matches the style of theelsebranch.♻️ Proposed flattening
if r.packageDeps != "" { pkgJSON, err := r.parsePackageJSON(filepath.Join(r.packageDeps, "package.json")) if err != nil { return nil, graph, fmt.Errorf("package-deps %s: %w", r.packageDeps, err) } - { - var ownerName string - if graph != nil { - for _, pkg := range r.workspacePackages { - if pkg.Path == r.packageDeps { - ownerName = pkg.Name - break - } - } - } - for depName := range pkgJSON.Dependencies { - if !workspaceNames[depName] { - allDeps[depName] = true - if graph != nil && ownerName != "" { - graph.AddDependency(ownerName, depName) - } - } - } - } + var ownerName string + if graph != nil { + for _, pkg := range r.workspacePackages { + if pkg.Path == r.packageDeps { + ownerName = pkg.Name + break + } + } + } + for depName := range pkgJSON.Dependencies { + if !workspaceNames[depName] { + allDeps[depName] = true + if graph != nil && ownerName != "" { + graph.AddDependency(ownerName, depName) + } + } + } } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resolve/local/local.go` around lines 578 - 596, The inner bare block that scopes ownerName is unnecessary—remove the `{ ... }` and inline the ownerName lookup so it shares the same scope as the rest of the logic; specifically locate the code around r.workspacePackages / r.packageDeps where ownerName is set, move the ownerName declaration out of the inner block (or extract a tiny helper like findOwnerName(pkgPath) if preferred), then iterate over pkgJSON.Dependencies and update allDeps, workspaceNames checks, and graph.AddDependency(ownerName, depName) as before but without the extra brace-scoped block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@resolve/local/local.go`:
- Around line 578-596: The inner bare block that scopes ownerName is
unnecessary—remove the `{ ... }` and inline the ownerName lookup so it shares
the same scope as the rest of the logic; specifically locate the code around
r.workspacePackages / r.packageDeps where ownerName is set, move the ownerName
declaration out of the inner block (or extract a tiny helper like
findOwnerName(pkgPath) if preferred), then iterate over pkgJSON.Dependencies and
update allDeps, workspaceNames checks, and graph.AddDependency(ownerName,
depName) as before but without the extra brace-scoped block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bbdb5f3-a4c1-49ab-b6c6-722e458f5b47
⛔ Files ignored due to path filters (4)
testdata/workspace-package-deps/node_modules/lit/package.jsonis excluded by!**/node_modules/**testdata/workspace-package-deps/node_modules/lodash/package.jsonis excluded by!**/node_modules/**testdata/workspace-serve-root/node_modules/@lit/reactive-element/package.jsonis excluded by!**/node_modules/**testdata/workspace-serve-root/node_modules/lit/package.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (16)
cmd/generate/generate.gonpm/src/mappa.tsresolve/local/local.goresolve/local/local_test.goresolve/local/options.gotestdata/workspace-package-deps/expected-all-deps.jsontestdata/workspace-package-deps/expected-core-deps.jsontestdata/workspace-package-deps/package.jsontestdata/workspace-package-deps/packages/core/package.jsontestdata/workspace-package-deps/packages/tools/package.jsontestdata/workspace-serve-root/examples/kitchen-sink/package.jsontestdata/workspace-serve-root/expected-no-rebase.jsontestdata/workspace-serve-root/expected.jsontestdata/workspace-serve-root/package.jsontestdata/workspace-serve-root/packages/lib/package.jsonwasm/main.go
Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
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
1 participant
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
Closes #38.
WithPathBase(dir)option to rebase workspace package paths relative to a given directory instead of the resolution root. Paths undernode_modules/are unaffected. This moves therebaseForServeRootworkaround from cem into mappa.WithPackageDeps(dir)option to limit dependency collection to only the dependencies listed in thepackage.jsonat the given directory, avoiding warnings for irrelevant workspace packages.--path-base,--package-deps), WASM/JS options, and theOptionsstruct.rootDir.ResolveIncrementalwhere workspace package writes were unprotected by the mutex.Test plan
TestResolverWithPathBase-- workspace paths rebased,node_modulesunchanged, scope keys preservedTestResolverWithPathBaseSameAsRoot-- no-op when path base equals resolution rootTestResolverWithPathBaseAutoDiscovery-- works with auto-discovered workspacesTestResolverWithPathBaseOutsideRoot-- no-op when path base is outside rootTestResolverWithRelativePathBase-- relative path normalized correctlyTestResolverWithPathBaseIncrementalIdempotent-- rebase is idempotent across incremental updatesTestResolverWithPackageDeps-- only scoped package's deps resolvedTestResolverWithPackageDepsFallback-- all deps whenpackageDepsunsetTestResolverWithPackageDepsError-- error returned for nonexistent pathTestResolverWithRelativePackageDeps-- relative path normalized correctlyTestResolverWithPathBaseAndPackageDeps-- combined usageTestResolverBuilderPropagation-- all fields survive builder method chainingTestOptionsApplyPathBaseAndPackageDeps--Options.ApplywiringAssisted-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit
Release Notes
New Features
Tests