fix(js): fix JSR doc-gen for libraries; exclude web-component packages#1733
Conversation
The first successful deno publish run got 5 libs onto JSR (token, json, loc, msf, hang) and exposed two more server-side issues that dry-run can't see: 1. JSR's doc generator can't resolve ".ts" import extensions (the dist ships .js/.d.ts) or a bare "." specifier in the published .d.ts. net used "./connection/index.ts" and signals' dom.ts imported from ".", so both failed doc generation. Normalize the emitted .d.ts: strip ".ts" extensions and turn "." into "./index". Only the .d.ts is touched; the .js keeps its specifiers. 2. JSR forbids modifying global types, and every custom element augments HTMLElementTagNameMap. So watch, publish, and boy fundamentally cannot publish to JSR. They opt out with "jsr": false; publishJsr now also honors that. With this, all 7 libraries pass `deno doc` locally (the same doc generation the publish performs) and the 3 web-component packages are excluded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR introduces a 🚥 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✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@js/common/release.ts`:
- Around line 8-11: The JSR gating logic in release.ts checks only `pkg.jsr !==
false` to determine whether to publish to JSR, but package.ts requires both a
release script AND `pkg.jsr !== false` before generating jsr.json. This mismatch
causes failures when a package lacks a release script but has no explicit `jsr:
false` setting. To align the gating logic across both files, remove the release
script requirement from the jsr.json generation condition in package.ts so that
jsr.json is generated whenever `pkg.jsr !== false`, regardless of whether a
release script exists. This ensures the publishJsr constant in release.ts will
only attempt JSR publishing when the necessary jsr.json file has been generated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a1b428d-b26c-4493-99f4-5fa6b209b471
📒 Files selected for processing (5)
js/common/package.tsjs/common/release.tsjs/moq-boy/package.jsonjs/publish/package.jsonjs/watch/package.json
Use the identical predicate (release script present AND jsr !== false) in both files so the generated manifest and the publish stay in lockstep. The release-script clause is always true in release.ts (it is the release script), so this is behavior-preserving; it just removes the apparent mismatch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR #1732 added an autolink to http://localhost:4443/certificate.sha256 in setup/windows.md, which VitePress can't reach at build time, failing the doc build (and every PR's `just ci`). Generalize the existing localhost exemption from a single :5173 literal to a regex covering any localhost URL. Folded in here so this PR is green on its own; supersedes #1734. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
doc/.vitepress/config.ts (1)
264-266: ⚡ Quick winExpand boundary check to handle query parameters and hash fragments.
The regex won't match localhost URLs with query parameters or hash fragments immediately after the port (e.g.,
http://localhost:5173?foo=barorhttp://localhost:5173#section). While uncommon in documentation, these are valid URLs.Changing
(\/|$)to(\/|\?|#|$)would match all valid localhost URLs while still preventing false positives likehttp://localhost.example.com.♻️ Suggested fix for more robust matching
- /^https?:\/\/localhost(:\d+)?(\/|$)/, + /^https?:\/\/localhost(:\d+)?(\/|\?|#|$)/,🤖 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 `@doc/.vitepress/config.ts` around lines 264 - 266, The regex pattern for matching localhost URLs in the development examples check currently uses (\/|$) which only matches a forward slash or end of string, but it doesn't account for query parameters or hash fragments. Modify the regex pattern by changing (\/|$) to (\/|\?|#|$) to match localhost URLs that may have query parameters (indicated by ?) or hash fragments (indicated by #) immediately after the port, while still maintaining the ability to prevent false positives like http://localhost.example.com.
🤖 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 `@doc/.vitepress/config.ts`:
- Around line 264-266: The regex pattern for matching localhost URLs in the
development examples check currently uses (\/|$) which only matches a forward
slash or end of string, but it doesn't account for query parameters or hash
fragments. Modify the regex pattern by changing (\/|$) to (\/|\?|#|$) to match
localhost URLs that may have query parameters (indicated by ?) or hash fragments
(indicated by #) immediately after the port, while still maintaining the ability
to prevent false positives like http://localhost.example.com.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bce50165-3b66-4220-9df3-453a8b615c18
📒 Files selected for processing (2)
doc/.vitepress/config.tsjs/common/release.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- js/common/release.ts
…heck Match a "?" or "#" right after the host/port too, so localhost example URLs with a query string or fragment are covered. The "." that would start localhost.example.com is still excluded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Where we are
The deno-based release (run 27509033095) published 5 libraries to JSR:
token,json,loc,msf,hang. It then exposed two more server-side issues (invisible todeno publish --dry-run, but reproducible withdeno doc):Issues fixed
1. Doc-gen can't resolve
.tsextensions or bare.in the published.d.ts.JSR generates the API docs by resolving the
.d.tsimport graph. The dist ships.js/.d.ts(no.ts), so:net'sexport * as Connection from "./connection/index.ts"→Failed resolving './connection/index.ts'signals'dom.tsimport ... from "."→Failed resolving './/'Fix: normalize the emitted
.d.tsinpackage.ts- strip.tsextensions and rewrite bare"."→"./index". Only declarations are touched; the.jskeeps its specifiers for npm consumers. (The successful libs already used extensionless imports, which is why they worked.)2. Web components fundamentally can't publish to JSR.
watch/publish/boyfail withmodifying global types is not allowed- every custom element augmentsHTMLElementTagNameMap, and JSR prohibits global type modification outright (--allow-slow-typesonly silenced the warning). This isn't fixable without dropping the custom-element typing, so these three opt out with"jsr": false.publishJsrnow honors that (still defaults on for everything else with areleasescript).Validation
All 7 libraries pass
deno doclocally - the exact doc generationdeno publishruns - across every entrypoint in their generatedjsr.json:The 3 web-component packages no longer generate a
jsr.json. Biome clean.After merge, the release should skip the 5 already-published libs, publish
net+signals, and skip the 3 web components → green, with all 7 libraries on JSR.Note on why this took several rounds
deno publish --dry-runvalidates packaging and slow types, but license validation and documentation generation only happen server-side at publish, so each surfaced only on a real run.deno docreproduces the doc-gen step locally and is now part of how these were verified.(Written by Claude)
🤖 Generated with Claude Code
Also (folded in from #1734)
The
Checkworkflow was failing on a pre-existing docs dead link from #1732 (http://localhost:4443/certificate.sha256insetup/windows.md). Generalized the VitePressignoreDeadLinkslocalhost exemption to a regex so dev-example URLs do not break the doc build. Folded here so this PR is green on its own; #1734 closed as superseded.