feat: upgrade QuotaExceededError to DOMException subclassfeat: upgrade QuotaExceededError to DOMException subclass#31440
Conversation
|
|
WalkthroughRemoves the Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
runtime/js/99_main.js (1)
305-315: formatException now appends an extra}to string error messagesThe updated template literal for string errors:
return `Uncaught ${inspectArgs([quoteString(error, getDefaultInspectOptions())], { colors: !getStderrNoColor(), }) }`;adds a literal
}(and whitespace/newline) after the inspected string. That changes user-visible output from e.g.:
Uncaught "boom"to something like:
Uncaught "boom"\n }This behavior change isn’t related to the QuotaExceededError work and will confuse users and tests that rely on the formatting.
You can restore the original intent with:
- return `Uncaught ${inspectArgs([quoteString(error, getDefaultInspectOptions())], { - colors: !getStderrNoColor(), - }) - }`; + return `Uncaught ${inspectArgs( + [quoteString(error, getDefaultInspectOptions())], + { colors: !getStderrNoColor() }, + )}`;
🧹 Nitpick comments (1)
ext/web/01_dom_exception.js (1)
196-234: QuotaExceededError subclass wiring and semantics look correctThe subclass correctly:
- Converts
messagevia WebIDL, fixesnameto"QuotaExceededError", and defaultsquota/requestedtonull.- Preserves legacy numeric constant
QUOTA_EXCEEDED_ERRvia the existing constants table while removing the name fromnameToCodeMapping, sonew DOMException("x", "QuotaExceededError").code === 0as desired.- Uses
webidl.configureInterface(QuotaExceededError)andassertBrandedconsistently withDOMException.If you want slightly better debuggability, consider extending the
Deno.privateCustomInspecthandler to includequotaandrequestedfor this subclass, but that’s optional and not required to meet the spec change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ext/web/01_dom_exception.js(1 hunks)runtime/js/99_main.js(4 hunks)tests/unit/dom_exception_test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/unit/dom_exception_test.tsext/web/01_dom_exception.jsruntime/js/99_main.js
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Lint code using `./tools/lint.js`
Applied to files:
runtime/js/99_main.js
⏰ 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). (7)
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: build libs
🔇 Additional comments (4)
tests/unit/dom_exception_test.ts (1)
34-68: Test coverage for QuotaExceededError behavior is solidThese tests accurately exercise the new behavior:
- Subclass chain (
QuotaExceededError→DOMException→Error),DOMException("...", "QuotaExceededError").code === 0,- Default
quota/requestednull values, and- Options-based initialization.
No additional scenarios are strictly necessary for this change set.
runtime/js/99_main.js (3)
79-79: Importing QuotaExceededError here is appropriateBringing
QuotaExceededErrorin alongsideDOMExceptionkeeps all DOM-related error builders sourced from the same module and enables the new subclass for quota errors without changing other call sites.
350-353: Using QuotaExceededError in the registered error builder is correctSwitching
DOMExceptionQuotaExceededErrorto constructnew QuotaExceededError(msg)cleanly aligns the runtime error path with the new subclass while preservinginstanceof DOMExceptionbehavior for consumers.
568-569: No-op Deno.test/bench placeholders remain behaviorally unchangedReformatting these no-op callbacks to
() => { }is purely stylistic and doesn’t affect their role as compatibility placeholders outsidedeno test/deno bench.
Implements Web IDL spec change (whatwg/webidl#1465) to upgrade QuotaExceededError from a DOMException name to a proper subclass. Changes: - Remove QuotaExceededError from DOMException names table - Create QuotaExceededError class extending DOMException - Add quota and requested properties (default to null) - Update error registration to use new QuotaExceededError class - Add comprehensive tests for QuotaExceededError subclass Fixes denoland#30028
Bitshifter-9
force-pushed
the
fix-quota-exceeded-error
branch
from
71b92f2 to
307c5ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/dom_exception_test.ts (1)
34-68: Tests look good, consider adding a test forQuotaExceededError.code.The tests cover the key behaviors. However, since the PR mentions a breaking change for
code, you might want to add a test verifying whatnew QuotaExceededError("msg").codereturns (should be22to matchQUOTA_EXCEEDED_ERR, or0per the new spec behavior). This would document the expected behavior explicitly.Deno.test(function quotaExceededErrorCodeValue() { const error = new QuotaExceededError("test message"); // Document expected code value for QuotaExceededError instances assertEquals(error.code, 0); // or 22, depending on intended behavior });ext/web/01_dom_exception.js (1)
196-229: Good implementation of the QuotaExceededError subclass.The class correctly extends DOMException, uses private fields, and follows the established patterns with
webidl.assertBranded.One consideration: per the WebIDL spec,
quotaandrequestedare typed asunsigned long long?. The current implementation assigns raw values without WebIDL type conversion. If strict spec compliance is desired:if (ObjectHasOwn(options, "quota")) { - this.#quota = options.quota ?? null; + this.#quota = options.quota === undefined || options.quota === null + ? null + : webidl.converters["unsigned long long"](options.quota, "Failed to construct 'QuotaExceededError'", "quota"); } if (ObjectHasOwn(options, "requested")) { - this.#requested = options.requested ?? null; + this.#requested = options.requested === undefined || options.requested === null + ? null + : webidl.converters["unsigned long long"](options.requested, "Failed to construct 'QuotaExceededError'", "requested"); }This is optional since the primary use case (internal error construction) will pass valid numbers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ext/web/01_dom_exception.js(1 hunks)runtime/js/99_main.js(4 hunks)tests/unit/dom_exception_test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/unit/dom_exception_test.tsext/web/01_dom_exception.jsruntime/js/99_main.js
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Lint code using `./tools/lint.js`
Applied to files:
runtime/js/99_main.js
⏰ 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). (8)
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test release macos-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
🔇 Additional comments (4)
runtime/js/99_main.js (3)
79-79: LGTM!Import correctly adds
QuotaExceededErroralongsideDOMException.
349-354: LGTM!The error builder now correctly returns a
QuotaExceededErrorinstance instead of constructing aDOMExceptionwith the name "QuotaExceededError". This aligns with the spec upgrade.
312-315: Whitespace-only formatting changes.These are cosmetic changes with no functional impact.
Also applies to: 568-569
ext/web/01_dom_exception.js (1)
231-234: LGTM!WebIDL interface configuration and exports are properly set up.
| // QuotaExceededError is now a subclass, not a DOMException name | ||
| // So creating a DOMException with name "QuotaExceededError" should have code 0 | ||
| const error = new DOMException("test", "QuotaExceededError"); | ||
| assertEquals(error.code, 0); |
There was a problem hiding this comment.
The WHATWG PR comment states the following, is this correct?
DOMException.QUOTA_EXCEEDED_ERR === 22(new DOMException("message", "QuotaExceededError")).code === DOMException.QUOTA_EXCEEDED_ERR
|
Thanks for the PR, gonna close this one since we landed #32244 which was a more featureful implementatation. |
4 participants
Implements Web IDL spec change (whatwg/webidl#1465) to upgrade QuotaExceededError from a DOMException name to a proper subclass.
Changes
ext/web/01_dom_exception.jsquotaandrequestedproperties (default to null)runtime/js/99_main.jsto use new QuotaExceededError classtests/unit/dom_exception_test.tsBreaking Change
new DOMException("message", "QuotaExceededError").codenow returns0instead of22to align with the Web IDL spec.Fixes #30028
cc @domenicImplements Web IDL spec change (whatwg/webidl#1465) to upgrade QuotaExceededError from a DOMException name to a proper subclass.
Changes:
Fixes #30028