fix(publish): honor jsr.json fallback and flag config conflicts #31383
Conversation
|
|
WalkthroughThis change adds workspace-level JSR package config handling into the publish flow and a new diagnostic for conflicting publish configs. It introduces maybe_jsr_package_config to locate, read, and (optionally strict-) validate jsr.json/jsr.jsonc at the workspace root. If both existing publish configs and a workspace JSR config are present, the code now aggregates specifiers, detects conflicts, and emits PublishDiagnostic::ConflictingPublishConfig before exiting. Tests were added for jsr reading, strict validation, non-strict behavior, and conflict diagnostics. The workspace dependency deno_maybe_sync was added to the workspace Cargo.toml. Sequence DiagramsequenceDiagram
actor User
participant PublishFlow as Publish Flow
participant ConfigResolver as Config Resolver
participant JsrHelper as maybe_jsr_package_config
participant Diagnostics as Diagnostics
User->>PublishFlow: start publish
PublishFlow->>ConfigResolver: load existing publish_configs
ConfigResolver-->>PublishFlow: publish_configs
PublishFlow->>JsrHelper: maybe_jsr_package_config(workspace_dir, strict?)
alt JSR config found
JsrHelper-->>PublishFlow: JsrPackageConfig
alt publish_configs non-empty
PublishFlow->>PublishFlow: aggregate specifiers (sort, dedupe)
PublishFlow->>Diagnostics: emit ConflictingPublishConfig(primary, specifiers)
Diagnostics-->>PublishFlow: error diagnostic
PublishFlow->>User: exit with conflict error
else publish_configs empty
PublishFlow->>PublishFlow: append JSR config to publish_configs
PublishFlow->>ConfigResolver: continue publish resolution
end
else JSR config not found
JsrHelper-->>PublishFlow: None
PublishFlow->>ConfigResolver: continue with original publish_configs
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)cli/tools/publish/mod.rs (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (4)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/tools/publish/mod.rs (1)
1573-1625: Tests cover core scenarios.The tests verify:
- Reading valid jsr.json successfully
- Strict mode validation (missing name errors)
Consider adding a test for non-strict mode behavior: verify that
maybe_jsr_package_configwithstrict=falsereturnsNone(rather than erroring) when jsr.json exists but lacks required fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
cli/Cargo.toml(1 hunks)cli/tools/publish/diagnostics.rs(10 hunks)cli/tools/publish/mod.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/tools/publish/diagnostics.rs (1)
cli/lsp/documents.rs (1)
primary_specifier(1437-1449)
cli/tools/publish/mod.rs (5)
libs/config/workspace/mod.rs (6)
workspace(2307-2311)workspace(2312-2312)default(1519-1521)dir_path(1717-1719)diagnostics(992-1185)empty(1549-1564)libs/maybe_sync/lib.rs (1)
new_rc(104-106)cli/factory.rs (2)
sys(517-519)default(251-253)libs/config/deno_json/mod.rs (2)
dir_path(1435-1441)read(1360-1370)cli/tools/publish/diagnostics.rs (2)
message(228-275)hint(430-477)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: build libs
🔇 Additional comments (7)
cli/Cargo.toml (1)
80-80: LGTM - Dependency is properly utilized.The
deno_maybe_syncdependency is used incli/tools/publish/mod.rs(line 31) for thenew_rcutility function that wraps config objects.cli/tools/publish/diagnostics.rs (2)
145-148: LGTM - New diagnostic variant added.The
ConflictingPublishConfigvariant is properly structured with both a primary specifier for error location and a full list of conflicting specifiers for the message.
200-200: LGTM - Complete and consistent diagnostic implementation.The new diagnostic is fully integrated across all trait methods (
level,code,message,location,snippet,hint,docs_url) following the existing patterns in this file.Also applies to: 222-224, 266-273, 347-351, 426-426, 473-475, 610-612
cli/tools/publish/mod.rs (4)
19-19: LGTM - Imports are properly used.All three imports are utilized in the new
maybe_jsr_package_configfunction and tests.Also applies to: 31-31, 58-58
92-119: Conflict detection logic is sound, but consider error handling clarity.The two-phase approach is correct:
- When
publish_configsexist (from deno.json), check for jsr.json conflicts without requiring it to be valid (strict=false)- When no publish configs exist, look for jsr.json and require it to be valid (
strict=true)However, note that when
strict=false(line 95), the function will still error on JSON parse errors or I/O errors - it only skips files that are valid JSON but lack required fields. This means if a user has a malformed jsr.json alongside a valid deno.json, publishing will fail. This might be intentional (invalid jsr.json should always be flagged), but consider if this is the desired UX.Verify this behavior is acceptable: Should a malformed jsr.json block publishing when deno.json is the intended config?
379-418: LGTM - Helper function correctly implements fallback logic.The function properly handles both strict and non-strict modes:
- Non-strict mode: Returns
Noneif jsr.json/jsr.jsonc don't exist or lack required fields (name/exports)- Strict mode: Errors if files exist but lack required fields
The iteration over both "jsr.json" and "jsr.jsonc" ensures .jsonc fallback works as expected.
1627-1640: LGTM - Diagnostic test verifies message formatting.The test confirms the diagnostic lists all conflicting specifiers in the message and provides an appropriate hint.
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
@coderabbitai I added a test as a reference to the nitpick comment, (8a3e817). Review it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1257 @Pinta365, the test looks good! It correctly addresses the nitpick by verifying that ✅ Creates a jsr.json with only a The implementation is clean and the test name clearly describes the expected behavior. This completes the test coverage for the non-strict mode fallback scenario. |
There was a problem hiding this comment.
This doesn't look like it will work when publishing a workspace. For example, someone might publish a workspace with members containing a deno.json and jsr.json -- Maybe we need more of a first class solution in libs/config instead of trying to fix this in the cli.
There was a problem hiding this comment.
For example, maybe we can get rid of additional_config_file_names and instead have a more specific discover_jsr_config: true flag that will surface a WorkspaceDiagnostic for this scenario.
Related is my comment here: https://github.com/denoland/deno/pull/31383/files#r2572813359 -- fixing that will help fix this.
There was a problem hiding this comment.
Ahh yes, I see what you mean with workspaces. This only works as a solution for the root package basically.
| ); | ||
| } | ||
|
|
||
| fn maybe_jsr_package_config( |
There was a problem hiding this comment.
maybe_package_config() on WorkspaceDirectory needs to be the source of truth for JsrPackageConfig. That way, someone using workspace_directory.maybe_package_config() will always get the right information (so, in other words, we should remove this function and make this work in WorkspaceDirectory.maybe_package_config())
For example, right now cli/tools/lint/mod.rs calls workspace_directory.maybe_package_config() and right now in this PR it's not getting the right information.
There was a problem hiding this comment.
I'll go back to the drawing table and come back with a suggested solution based on this input. Thanks for the review so far.
3 participants
Summary
deno publishfall back tojsr.json(c)when no publishabledeno.json(c)exists, so packages that keep their metadata injsr.jsonwork as expectedconflicting-publish-configdiagnostic (with a hint and docs link) when both config files define package metadata, so authors resolve the ambiguity before publishingFixes #22651.
Testing
cargo test -p deno -- maybe_jsr_package_config./tools/format.js./tools/lint.js