Repository navigation
feat(hot): let the client say whose runtime it is in the console - #2462
Conversation
🦋 Changeset detectedLatest commit: e1a55f6 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 configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. WalkthroughThe client logging option now accepts a level string or an object with a level and optional name. The client parses JSON logging overrides and applies a truthy name to console messages. The logger uses the package default when no name is set. The change also updates the option schema, TypeScript declarations, documentation, and end-to-end tests for the custom label. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The custom logger name reaches the client while preserving the configured logging level. No actionable merge-blocking risk remains in the reviewed change. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The new name controls the hot client's console label, not its authentication, connection destination, or permissions. Existing logging levels and the default label remain supported. No material security risk was identified in the changed behavior. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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:
87558674-7e06-4e24-a43d-a02a9c59183f
📒 Files selected for processing (9)
.changeset/client-logger-name.mdREADME.mdclient-src/index.jsclient-src/utils/log.jssrc/options.check.jssrc/options.jsontest/e2e/messages.test.jstypes/client/index.d.tstypes/client/utils/log.d.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2462 +/- ##
===========================================
+ Coverage 67.55% 96.31% +28.75%
===========================================
Files 23 23
Lines 2484 2494 +10
===========================================
+ Hits 1678 2402 +724
+ Misses 806 92 -714 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Include the object form in HotClientOptions.logging. · options.json:448-481
src/options.json:448-481
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winInclude the object form in
HotClientOptions.logging.
Options.hotusesHotOptions, whoseclientproperty usesHotClientOptions. That type accepts only level strings, so TypeScript rejects the documentedhot.client.logging: { level, name }configuration. Update the source JSDoc and its published declaration.Suggested fix
--- a/src/hot.js +++ b/src/hot.js @@ - * @property {("none" | "error" | "warn" | "info" | "log" | "verbose")=} logging how much the runtime logs to the browser console + * @property {(("none" | "error" | "warn" | "info" | "log" | "verbose") | { level?: ("none" | "error" | "warn" | "info" | "log" | "verbose"), name?: string })=} logging how much the runtime logs to the browser console --- a/types/hot.d.ts +++ b/types/hot.d.ts @@ - logging?: - ("none" | "error" | "warn" | "info" | "log" | "verbose") | undefined; + logging?: + | ("none" | "error" | "warn" | "info" | "log" | "verbose") + | { + level?: + | ("none" | "error" | "warn" | "info" | "log" | "verbose") + | undefined; + name?: string | undefined; + } + | undefined;
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
43a5e188-605b-43e3-a69c-71c51aa01558
📒 Files selected for processing (2)
client-src/index.jstest/e2e/messages.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/messages.test.js
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review.
`hot.client.logging` takes an object as well as a level, carrying the name
every message is labelled with:
hot: { client: { logging: { level: "warn", name: "my-dev-server" } } }
The reason is the one that made the overlay's element id an option. The
package a developer installed is the one they would report a problem to, and a
console labelled with a dependency's name sends them to the wrong repository.
It matters most where this runtime is the whole of another package's client —
webpack-dev-server, whose users have read `[webpack-dev-server]` for years and
would otherwise start reading this package's name instead.
Unset, it is this package's name as before.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
The second shape was detected by testing the first character for `{`, so json
with a space before it fell through: the object text became the level and the
name was lost. Asking what `JSON.parse` returned instead covers that, and
covers more than trimming would — `JSON.parse` also accepts a bare number,
boolean or quoted string, and a level is none of those, so requiring an object
is the check that was meant.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
dc4744e to
b76449f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Add the object form to HotClientOptions.logging. · options.json:448-481
src/options.json:448-481
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd the object form to
HotClientOptions.logging.When a TypeScript configuration sets
hot.client.loggingto{ level: "info", name: "my-dev-server" },HotOptions.clientresolves toHotClientOptions, whoseloggingtype accepts only strings. The runtime schema accepts this object, but the exported type rejects it. Add the object variant with optionallevelandname.Suggested fix
logging?: - ("none" | "error" | "warn" | "info" | "log" | "verbose") | undefined; + | ("none" | "error" | "warn" | "info" | "log" | "verbose") + | { + level?: "none" | "error" | "warn" | "info" | "log" | "verbose" | undefined; + name?: string | undefined; + } + | undefined;
🧹 Nitpick comments (1)
test/e2e/messages.test.js (1)
151-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover filtering for object-form levels.
Both object-form browser tests use
"info", the client default, and assert the custom logger name. Neither would catch a change that applies the object’s name but ignores itslevel. Add an object-form"warn"browser test that asserts filtering.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
1f63b204-db05-4d35-98a1-a9294483b2c9
📒 Files selected for processing (1)
test/e2e/messages.test.js
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
The runtime schema accepted `{ level, name }` while the generated `.d.ts` only
accepted a level, so a TypeScript configuration setting the object form got a
type error for something that works. The level is a `LogLevel` alias now,
which both halves of the union share.
Also a case the two browser tests could not catch: both used `"info"`, the
default, so the object's `name` being applied while its `level` was dropped
would have passed them. The new one asks for `"warn"` and requires `connected`
— logged at info — to be absent, so the level has to come from inside the
object. Checked by breaking it on purpose: it fails.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
|
Both out-of-diff findings addressed in
|
DO NOT MERGE THIS COMMIT. Revert it, and set the dependency back to a released `^8.4.0`, once webpack-dev-middleware ships the changes below. This branch needs APIs the published 8.3.0 does not have (`onConnect` with the request, `handleUpgrade`, `publish`, `publishTo`, `hot.client.path` as parts, `hot.client.logging` with a name, an `error` action). CI installs from the registry, so `tsc` failed in the Build step and every job behind it was cancelled — which says nothing about whether the code works. This vendors a pack of the middleware as it will be once these land: - webpack/webpack-dev-middleware#2459 (client path in parts) - webpack/webpack-dev-middleware#2462 (logger name) on top of `main`, which already carries #2458, #2460 and #2461. The lockfile change is confined to that one dependency and its own metadata. To undo, with the release out: git revert <this commit> npm install webpack-dev-middleware@^8.4.0 Squash-merging also drops it, but only if the branch's last state is clean. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
DO NOT MERGE THIS COMMIT. Revert it, and set the dependency back to a released `^8.4.0`, once webpack-dev-middleware ships the changes below. This branch needs APIs the published 8.3.0 does not have (`onConnect` with the request, `handleUpgrade`, `publish`, `publishTo`, `hot.client.path` as parts, `hot.client.logging` with a name, an `error` action). CI installs from the registry, so `tsc` failed in the Build step and every job behind it was cancelled — which says nothing about whether the code works. This vendors a pack of the middleware as it will be once these land: - webpack/webpack-dev-middleware#2459 (client path in parts) - webpack/webpack-dev-middleware#2462 (logger name) - webpack/webpack-dev-middleware#2463 (says when the connection goes away) on top of `main`, which already carries #2458, #2460 and #2461. The lockfile change is confined to that one dependency and its own metadata. To undo, with the release out: git revert <this commit> npm install webpack-dev-middleware@^8.4.0 Squash-merging also drops it, but only if the branch's last state is clean. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
DO NOT MERGE THIS COMMIT. Revert it, and set the dependency back to a released `^8.4.0`, once webpack-dev-middleware ships the changes below. This branch needs APIs the published 8.3.0 does not have (`onConnect` with the request, `handleUpgrade`, `publish`, `publishTo`, `hot.client.path` as parts, `hot.client.logging` with a name, an `error` action). CI installs from the registry, so `tsc` failed in the Build step and every job behind it was cancelled — which says nothing about whether the code works. This vendors a pack of the middleware as it will be once these land: - webpack/webpack-dev-middleware#2459 (client path in parts) - webpack/webpack-dev-middleware#2462 (logger name) - webpack/webpack-dev-middleware#2463 (says when the connection goes away) on top of `main`, which already carries #2458, #2460 and #2461. The lockfile change is confined to that one dependency and its own metadata. To undo, with the release out: git revert <this commit> npm install webpack-dev-middleware@^8.4.0 Squash-merging also drops it, but only if the branch's last state is clean. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
DO NOT MERGE THIS COMMIT. Revert it, and set the dependency back to a released `^8.4.0`, once webpack-dev-middleware ships the changes below. This branch needs APIs the published 8.3.0 does not have (`onConnect` with the request, `handleUpgrade`, `publish`, `publishTo`, `hot.client.path` as parts, `hot.client.logging` with a name, an `error` action). CI installs from the registry, so `tsc` failed in the Build step and every job behind it was cancelled — which says nothing about whether the code works. This vendors a pack of the middleware as it will be once these land: - webpack/webpack-dev-middleware#2459 (client path in parts) - webpack/webpack-dev-middleware#2462 (logger name) - webpack/webpack-dev-middleware#2463 (says when the connection goes away) on top of `main`, which already carries #2458, #2460 and #2461. The lockfile change is confined to that one dependency and its own metadata. To undo, with the release out: git revert <this commit> npm install webpack-dev-middleware@^8.4.0 Squash-merging also drops it, but only if the branch's last state is clean. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
DO NOT MERGE THIS COMMIT. Revert it, and set the dependency back to a released `^8.4.0`, once webpack-dev-middleware ships the changes below. This branch needs APIs the published 8.3.0 does not have (`onConnect` with the request, `handleUpgrade`, `publish`, `publishTo`, `hot.client.path` as parts, `hot.client.logging` with a name, an `error` action). CI installs from the registry, so `tsc` failed in the Build step and every job behind it was cancelled — which says nothing about whether the code works. This vendors a pack of the middleware as it will be once these land: - webpack/webpack-dev-middleware#2459 (client path in parts) - webpack/webpack-dev-middleware#2462 (logger name) - webpack/webpack-dev-middleware#2463 (says when the connection goes away) on top of `main`, which already carries #2458, #2460 and #2461. The lockfile change is confined to that one dependency and its own metadata. To undo, with the release out: git revert <this commit> npm install webpack-dev-middleware@^8.4.0 Squash-merging also drops it, but only if the branch's last state is clean. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
1 participant
Fourth prerequisite found while migrating webpack-dev-server onto this middleware's hot runtime.
Why
LOGGER_NAMEwas hardcoded to this package's name, so every console message from the runtime reads[webpack-dev-middleware] …. That is right when this middleware is what someone installed, and wrong when the runtime is the whole of another package's client.webpack-dev-server is that case: its users have read
[webpack-dev-server] …for years, and four of its snapshots record exactly that — including[webpack-dev-server] Invalid Host/Origin header, a line people search for. Taking this runtime as-is would relabel all of it and send anyone with a problem to the wrong repository.This is the same reasoning that made the overlay's element id an option: the package a developer installed is the one they would report to.
The option
hot.client.loggingtakes an object as well as a level — the scalar-or-object idiomoverlay,connect,cors,token,progressand nowpathall use, so no new option name:Messages read
[my-dev-server] …. Unset, it is this package's name exactly as before.Verified
npm run lint— clean (eslint, prettier, cspell,tsc, client types, schema-check)[my-dev-server]and not[webpack-dev-middleware], so a regression is visible rather than cosmeticIndependent of #2459, #2460 and #2461; all four are prerequisites for webpack/webpack-dev-server#5750 and can land in any order.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Generated by Claude Code
Summary by CodeRabbit
hot.client.loggingaccepts a logging-level string or an object with an optional level and name. Set a name to label console messages; when omitted, the package name is used.