fix(client): ship types for the client exports, and say it is ESM - #2428
alexander-akait wants to merge 2 commits into
Conversation
Found while starting the webpack-dev-server side of the client move: none of the five `./client/*` exports could be imported by a TypeScript consumer. `tsconfig.client.json` only ever checked the client, never emitted for it, so the exports carried no `types` condition and no declarations existed to point one at. Importing `webpack-dev-middleware/client/ws` failed outright under `noImplicitAny` (TS7016). Declarations alone were not enough. The client keeps ES module syntax — babel runs with `modules: false` so webpack can tree-shake it — but the package is CommonJS and nothing said otherwise, so under `node16`/`nodenext` resolution both the code and the new declarations resolved as CommonJS and the default import came back as the module namespace, which is not a constructor. A nested `package.json` in `client` and `types/client` states what those directories actually contain. No published path changes, which matters because `client.webSocketTransport` points at one of them. Verified from the consumer that reported it: webpack-dev-server's `lint:types-client` failed on the import before, and passes against a pack of this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
🦋 Changeset detectedLatest commit: a552101 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review. WalkthroughThe change adds TypeScript declarations for client connection APIs, HMR functions, overlays, indicators, and utility modules. The type build now emits client declarations and marks the client packages as ES modules. Package exports for five client subpaths now provide conditional declaration and runtime JavaScript targets. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The client declaration and export changes appear ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new declarations make existing client exports usable by TypeScript consumers, and the ES-module markers change how those files are resolved. No new attacker-controlled runtime path was demonstrated. Build ordering and incomplete downstream coverage leave some packaging uncertainty. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 3c2d5b6d-8397-4b04-928c-5c5bdfc5926b
📒 Files selected for processing (18)
.changeset/client-type-declarations.mdpackage.jsonscripts/mark-client-esm.mjstsconfig.client.build.jsontypes/client/clients/EventSourceClient.d.tstypes/client/clients/WebSocketClient.d.tstypes/client/clients/createSocket.d.tstypes/client/index.d.tstypes/client/indicator.d.tstypes/client/overlay.d.tstypes/client/package.jsontypes/client/process-update.d.tstypes/client/theme.d.tstypes/client/utils/get-hot.d.tstypes/client/utils/log.d.tstypes/client/utils/reload.d.tstypes/client/utils/send-message.d.tstypes/client/utils/strip-ansi.d.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
Review finding, and reproducible: `build` runs every `build:*` in parallel, so `build:types` can reach the marker script before babel has created `client`, and the write fails with `ENOENT`. It went unnoticed because `tsc` is the slower of the two here, so babel always happened to win — and CI happened to agree. `build:types` on its own after a `clean` fails every time rather than occasionally, which is what made it easy to confirm and now easy to check: that sequence errored before this and exits 0 after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2428 +/- ##
=======================================
Coverage 95.91% 95.91%
=======================================
Files 18 18
Lines 2006 2006
=======================================
Hits 1924 1924
Misses 82 82 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
Found by starting the webpack-dev-server side of the client move: none of the five
./client/*exports can be imported by a TypeScript consumer.tsconfig.client.jsononly ever checked the client (noEmit: true) and never emitted for it, so no declarations existed and the exports carried notypescondition.import WebSocketClient from "webpack-dev-middleware/client/ws"failed outright undernoImplicitAnywith TS7016.Declarations alone did not fix it. The client keeps ES module syntax — babel runs with
modules: falseso webpack can tree-shake it — but the package is CommonJS and nothing says otherwise. Undernode16/nodenextresolution both the code and the new declarations therefore resolve as CommonJS, and the default import comes back as the module namespace rather than the class:So this adds both halves:
tsconfig.client.build.jsonemits declarations forclient-srcintotypes/client, wired intobuild:types.typesconditions on./client,./client/sse,./client/ws,./client/indicatorand./client/overlay.package.jsonmarkingclientandtypes/clientas"type": "module", which is what they have always contained.No published path changes, which matters:
client.webSocketTransportpoints atclient/clients/WebSocketClient.js, so renaming to.mjswas not an option.Verified from the consumer that reported it, not just locally: webpack-dev-server's
lint:types-clientfails on that import against published 8.3.0 and passes against a pack of this branch. That is the check this is for, so it seemed worth proving rather than asserting.What kind of change does this PR introduce?
fix — packaging and types
Did you add tests for your changes?
No new test here; the declarations are build output and what exercises them is a consumer's typecheck, which is where it was verified. If you would like it guarded in CI, the smallest thing that would work is a fixture that imports each
./client/*export undernodenextand typechecks it — say the word and I will add one.Verified: full build, lint and both typechecks clean, non-browser 6819 passed, end-to-end 110/110.
test/logging.test.jsfails 74/74 on a cleanmaintoo, unrelated to this.Does this PR introduce a breaking change?
No. Nothing that resolved before stops resolving: the exports keep their existing paths and gain a
typescondition, and the"type": "module"marker states what those directories already were — arequire()of them never worked, since they are ES modules.If relevant, what needs to be documented once your changes are merged or what have you already documented?
Nothing new — the README already shows these imports in ESM form. This makes that form actually typecheck.
Use of AI
AI-assisted (Claude Code). Used to diagnose why the import failed in a real consumer, work out that declarations alone were not sufficient under
nodenext, implement both halves, and verify by packing this branch into webpack-dev-server and re-running its client typecheck.🤖 Generated with Claude Code
https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Generated by Claude Code
Summary by CodeRabbit