feat: add Intl.RelativeTimeFormat/PluralRules and named window access - #50
Merged
Conversation
Closes the two Intl gaps left unmodeled in v0.18 (RelativeTimeFormat, PluralRules) — directly relevant to the project's own countdown-timer and pluralized-greeting examples. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Elements with an id, or a name attribute on embed/form/img/object/ iframe/frame, become reachable as bare globals while connected to the primary document — a common pattern in real-world CMS-embedded widget scripts that predate/skip getElementById/querySelector. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds ChangesIntl formatting APIs
Window named access
Release metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DOMMutationAPI
participant NamedPropertiesRegistry
participant globalThis
DOMMutationAPI->>NamedPropertiesRegistry: connect, remove, or rename element
NamedPropertiesRegistry->>NamedPropertiesRegistry: update element ownership
NamedPropertiesRegistry->>globalThis: assign element, array, or undefined
Possibly related PRs
Suggested labels: 🚥 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: 6
🧹 Nitpick comments (2)
example/src/__harness__/JSDOM.namedaccess.harness.ts (1)
31-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every advertised
name-eligible element.This test title claims
embed,object,iframe, andframesupport, but only assertsformandimg. Add fixtures and assertions for the remaining four types so a partial registry implementation cannot pass the harness.🤖 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 `@example/src/__harness__/JSDOM.namedaccess.harness.ts` around lines 31 - 50, Extend the named-access harness test around the existing form/img fixtures to include embed, object, iframe, and frame elements with distinct name attributes. Add corresponding evaluate-result fields and expected tag-name assertions for each, while retaining the span assertion to verify arbitrary elements remain excluded.cpp/quickjs/bindings/WindowNamedPropertiesBindings.cpp (1)
138-143: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
insertAdjacentHTMLre-walks the whole subtree on every call.For
beforebegin/afterendthis callsregisterSubtree(this.parentNode), which runsquerySelectorAll('*')over the entire parent subtree (not just the newly-inserted nodes) on every invocation;afterbegin/beforeendsimilarly re-walks all ofthis. TheindexOfguard inaddToRegistrykeeps this idempotent, but it's O(subtree size) per call rather than O(inserted nodes), which can matter on large trees or hot insertion loops.🤖 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 `@cpp/quickjs/bindings/WindowNamedPropertiesBindings.cpp` around lines 138 - 143, Update the insertAdjacentHTML override to register only the nodes newly inserted by the operation, rather than re-walking this or its parent subtree via registerSubtree. Capture the relevant insertion boundary before and after calling origInsertAdjacentHTML, then register the inserted element nodes while preserving correct handling for beforebegin, afterend, afterbegin, and beforeend positions.
🤖 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 `@cpp/quickjs/bindings/IntlBindings.cpp`:
- Around line 300-305: In the numeric:auto lookup within IntlBindings.cpp,
remove rounding and use String(value) so idiomatic labels apply only to exact
supported values; otherwise preserve numeric formatting. Add a fractional-value
assertion in example/src/__harness__/JSDOM.intl.harness.ts (lines 224-251), such
as format(1.2, 'day'), verifying it falls back to numeric output.
In `@cpp/quickjs/bindings/IntlBindings.hpp`:
- Around line 18-20: Update the comment near the Intl relative-time formatting
behavior to document that RTF_AUTO also maps second: 0 to the localized
“now”/“agora” phrase, alongside the existing day/week/month/year auto-phrasing
description.
In `@cpp/quickjs/bindings/WindowNamedPropertiesBindings.cpp`:
- Around line 109-119: Update the removeChild and remove wrappers to capture the
subtree’s unregister keys before invoking the native operation, but defer
registry/globalThis mutation until that operation completes successfully. Ensure
removeChild leaves the registry unchanged when origRemoveChild.call throws,
while Element.prototype.remove preserves its no-op behavior for unparented
elements and commits unregistration only after successful removal.
- Around line 39-91: Update the WindowNamedPropertiesBindings bootstrap to
intercept the element textContent assignment path and unregister the element’s
existing descendant subtree before delegating to the native setter, then allow
normal registration tracking afterward. Reuse unregisterSubtree and the existing
inner* setter monkey-patch pattern; do not rely on native invalidation to remove
stale registry entries.
In `@cpp/quickjs/DOMBindings.cpp`:
- Line 72: Update the binding installation order around
WindowNamedPropertiesBindings::install so named-property handling cannot claim
or delete the window’s localStorage or sessionStorage properties. Preserve the
required ordering relative to ElementBindings and CustomElementsBindings while
ensuring storage bindings are installed or protected before named-access
processing.
In `@example/src/__harness__/JSDOM.namedaccess.harness.ts`:
- Around line 132-146: Update duplicate named-access handling in the relevant
binding so multiple matches return a live HTMLCollection rather than a plain
Array, preserving item(), namedItem(), liveness, and HTMLCollection type
behavior. Revise the test case “duplicate ids resolve to a plain array of the
matching elements” to assert HTMLCollection semantics and retain verification of
both matched elements.
---
Nitpick comments:
In `@cpp/quickjs/bindings/WindowNamedPropertiesBindings.cpp`:
- Around line 138-143: Update the insertAdjacentHTML override to register only
the nodes newly inserted by the operation, rather than re-walking this or its
parent subtree via registerSubtree. Capture the relevant insertion boundary
before and after calling origInsertAdjacentHTML, then register the inserted
element nodes while preserving correct handling for beforebegin, afterend,
afterbegin, and beforeend positions.
In `@example/src/__harness__/JSDOM.namedaccess.harness.ts`:
- Around line 31-50: Extend the named-access harness test around the existing
form/img fixtures to include embed, object, iframe, and frame elements with
distinct name attributes. Add corresponding evaluate-result fields and expected
tag-name assertions for each, while retaining the span assertion to verify
arbitrary elements remain excluded.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b40da912-4d4d-4635-8fc7-1730de912601
⛔ Files ignored due to path filters (1)
example/ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
CHANGELOG.mdandroid/CMakeLists.txtcpp/quickjs/DOMBindings.cppcpp/quickjs/bindings/IntlBindings.cppcpp/quickjs/bindings/IntlBindings.hppcpp/quickjs/bindings/WindowNamedPropertiesBindings.cppcpp/quickjs/bindings/WindowNamedPropertiesBindings.hppdocs/overview.mdexample/src/__harness__/JSDOM.intl.harness.tsexample/src/__harness__/JSDOM.namedaccess.harness.ts
💤 Files with no reviewable changes (1)
- CHANGELOG.md
format(1.2, 'day') was rounding to 1 before checking the idiomatic- phrase map, incorrectly returning "tomorrow" instead of falling through to numeric output. Address CodeRabbit review on PR #50. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Hook element.textContent: the native setter destroys old children without going through removeChild, so ids/names on those children were left stale in the registry. - removeChild()/remove() now check parentNode before committing the unregistration, instead of unregistering unconditionally: both silently no-op on the wrong parent/no parent, so a still-connected element no longer disappears from named access. - Install WindowNamedPropertiesBindings after localStorage/ sessionStorage are set up, so it can never claim (and later delete) those globals. Address CodeRabbit review on PR #50. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
<frame> inside <body> is dropped by HTML parsing (only valid inside <frameset>), so the parsed-HTML fixture never actually created the element. Test it via createElement + appendChild instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example/src/__harness__/JSDOM.namedaccess.harness.ts (1)
209-217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not codify
insertBefore()as an accepted limitation.
insertBefore()connects the element to the document, so this test makesnotHookedunreachable in the harness while contradicting the named-access contract established byappendChild(). HookinsertBefore()in the named-properties bootstrap and assert thatnotHooked === el, or remove the behavior from the named-access contract entirely.🤖 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 `@example/src/__harness__/JSDOM.namedaccess.harness.ts` around lines 209 - 217, Update the named-properties bootstrap and the related harness test so insertBefore() registers inserted elements consistently with appendChild(); change the test to retain the inserted element and assert that notHooked resolves to that element, rather than documenting it as unsupported.
♻️ Duplicate comments (1)
example/src/__harness__/JSDOM.namedaccess.harness.ts (1)
171-185: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not lock duplicate named access to a plain
Array.This repeats the previously reported contract issue: HTML-spec named access should expose a live
HTMLCollectionfor multiple matches, while the current test requiresArray.isArray(dup). Update the binding and assertions to coverHTMLCollectionbehavior (item(),namedItem(), and liveness).🤖 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 `@example/src/__harness__/JSDOM.namedaccess.harness.ts` around lines 171 - 185, Update the duplicate named-access implementation and the test around the duplicate `dup` fixture so multiple matches return a live `HTMLCollection`, not a plain array. Replace the `Array.isArray` expectation with assertions for `item()` and `namedItem()`, and verify the collection reflects subsequent DOM changes while preserving element order and length updates.
🧹 Nitpick comments (1)
example/src/__harness__/JSDOM.intl.harness.ts (1)
224-240: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the month and year auto phrases promised by this test.
The test title claims
day/week/month/yearcoverage, but it only exercises day and week. Add at least one month and one year assertion so regressions in the correspondingRTF_AUTOentries are detected.🤖 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 `@example/src/__harness__/JSDOM.intl.harness.ts` around lines 224 - 240, Extend the Intl.RelativeTimeFormat test in the evaluate payload to assert at least one numeric:"auto" month phrase and one year phrase, alongside the existing day/week assertions. Add corresponding result fields with expected localized outputs so the RTF_AUTO month and year entries are covered.
🤖 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.
Outside diff comments:
In `@example/src/__harness__/JSDOM.namedaccess.harness.ts`:
- Around line 209-217: Update the named-properties bootstrap and the related
harness test so insertBefore() registers inserted elements consistently with
appendChild(); change the test to retain the inserted element and assert that
notHooked resolves to that element, rather than documenting it as unsupported.
---
Duplicate comments:
In `@example/src/__harness__/JSDOM.namedaccess.harness.ts`:
- Around line 171-185: Update the duplicate named-access implementation and the
test around the duplicate `dup` fixture so multiple matches return a live
`HTMLCollection`, not a plain array. Replace the `Array.isArray` expectation
with assertions for `item()` and `namedItem()`, and verify the collection
reflects subsequent DOM changes while preserving element order and length
updates.
---
Nitpick comments:
In `@example/src/__harness__/JSDOM.intl.harness.ts`:
- Around line 224-240: Extend the Intl.RelativeTimeFormat test in the evaluate
payload to assert at least one numeric:"auto" month phrase and one year phrase,
alongside the existing day/week assertions. Add corresponding result fields with
expected localized outputs so the RTF_AUTO month and year entries are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bb695d48-a9af-4cef-83fe-8f01878ae2df
📒 Files selected for processing (7)
cpp/quickjs/DOMBindings.cppcpp/quickjs/bindings/IntlBindings.cppcpp/quickjs/bindings/IntlBindings.hppcpp/quickjs/bindings/WindowNamedPropertiesBindings.cppcpp/quickjs/bindings/WindowNamedPropertiesBindings.hppexample/src/__harness__/JSDOM.intl.harness.tsexample/src/__harness__/JSDOM.namedaccess.harness.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- cpp/quickjs/DOMBindings.cpp
- cpp/quickjs/bindings/IntlBindings.hpp
- cpp/quickjs/bindings/WindowNamedPropertiesBindings.hpp
- cpp/quickjs/bindings/WindowNamedPropertiesBindings.cpp
- cpp/quickjs/bindings/IntlBindings.cpp
|
🎉 This PR is included in version 2.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Intl.RelativeTimeFormatandIntl.PluralRules(pure-JS, en/pt locale data), closing the twoIntlgaps flagged as unmodeled in v0.18 — directly relevant to the project's countdown-timer and pluralized-greeting examples.id(or anameonembed/form/img/object/iframe/frame) become reachable as bare globals while connected to the document — a common pattern in real-world CMS-embedded widget scripts.Test plan
npx tsc --noEmitinexample/passesyarn test:harness:iospasses (361 tests, including newJSDOM.intl.harness.tsandJSDOM.namedaccess.harness.tscases)yarn test:harness:android🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Intl.PluralRulessupport, including locale-aware plural-category selection andresolvedOptions().Intl.RelativeTimeFormatsupport with localized unit names andnumericmodes (includingautobehavior).idsupport).Documentation
Tests