Support absolute targetPackagePath independent of cwd - #204
Merged
Conversation
A relative targetPackagePath keeps its existing semantics: resolved against the current working directory, which is assumed to be the workspace root. An absolute path previously produced a broken join against cwd; it now targets that directory directly and resolves the workspace root like the no-targetPackagePath case does, from the workspaceRoot setting or by walking upward from the target package. This lets callers such as firebase-tools invoke isolate() correctly regardless of the directory the process runs from.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, preserves existing relative-path behavior, and includes direct unit test coverage for the new absolute-path semantics and key edge cases.
Pull request overview
This PR updates resolveWorkspacePaths so an absolute targetPackagePath works correctly regardless of the current working directory, resolving the workspace root via workspaceRoot (when set) or via auto-detection from the target package directory—matching the behavior of the no-targetPackagePath case.
Changes:
- Support absolute
targetPackagePathwithout incorrectly pinningworkspaceRootDirtoprocess.cwd(). - Add unit tests covering relative vs absolute
targetPackagePathbehavior, includingworkspaceRoothonoring and failure mode when auto-detection can’t find a root. - Update configuration documentation to describe the relative/absolute semantics clearly.
File summaries
| File | Description |
|---|---|
src/lib/config.ts |
Adjusts workspace path resolution logic to treat absolute targetPackagePath as cwd-independent and to honor/auto-detect workspaceRootDir accordingly. |
src/lib/config.test.ts |
Adds targeted tests for resolveWorkspacePaths covering new absolute-path behavior and existing relative-path behavior. |
docs/configuration.md |
Documents the new absolute targetPackagePath behavior and clarifies when workspaceRoot is ignored vs honored. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
targetPackagePathwas resolved withpath.join(process.cwd(), targetPackagePath)and the workspace root pinned toprocess.cwd(), so callers had to guarantee the process runs at the workspace root. An absolute path produced a broken join.An absolute
targetPackagePathnow targets that directory directly, and the workspace root is resolved the same way as the no-targetPackagePathcase: from theworkspaceRootsetting, or auto-detected by walking upward from the target package directory. Relative paths keep their exact existing semantics, so this is purely additive — absolute paths were previously a broken input nobody could depend on.Motivation: the firebase-tools PR upstreaming the isolate integration (firebase/firebase-tools#10996) got review feedback that its isolate call breaks when the CLI runs from a subdirectory of the project. With this change firebase-tools can pass its already-resolved absolute source directory and be correct from any cwd, without a
process.chdirworkaround.Decisions:
path.isAbsoluterather than changing relative-path semantics, to stay non-breaking for existing configs.workspaceRootis honored instead of ignored (it was only ignored before because cwd was assumed to be the root).