fix(checkbox): rename internal id field to avoid createElement error - #4191
Open
adamczykpiotr wants to merge 1 commit into
Open
fix(checkbox): rename internal id field to avoid createElement error#4191adamczykpiotr wants to merge 1 commit into
adamczykpiotr wants to merge 1 commit into
Conversation
The component held a private field literally named `id`
(`private id = createRandomString()`). Because the component extends
HTMLElement, `id` shadows the native `HTMLElement.prototype.id` accessor.
Compiled with `useDefineForClassFields: false`, the field initializer runs
in the constructor as `this.id = …`, invoking the native `id` setter, which
adds an `id` attribute to the element during construction.
Per the DOM spec, a custom element constructed synchronously via
`document.createElement()`/`createElementNS()` must be pristine (no
attributes/children); otherwise the browser throws
`NotSupportedError: The result must not have attributes`. Frameworks that
construct elements eagerly hit this — e.g. Vue's renderer calls
`document.createElement('limel-checkbox')` on the already-defined tag, so it
upgrades synchronously, the constructor adds `id`, and it throws. The checkbox
then never hydrates. The Stencil lazy-load / parser path is unaffected, which
is why this wasn't caught before.
Rename the field to `inputId` (consistent with the sibling `helperTextId`) so
it no longer shadows the native property. Behaviour of the internal
input/label wiring is unchanged; the host element no longer gets a stray
generated `id`, and a consumer-provided host `id` is now left untouched.
Fixes #4190
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe checkbox’s generated input identifier is renamed to avoid shadowing the native host ChangesCheckbox id handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4191/ |
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 by CodeRabbit
Fixes #4190
What
limel-checkboxheld aprivatefield literally namedid:Because the component extends
HTMLElement,idshadows the nativeHTMLElement.prototype.idaccessor. Compiled with the defaultuseDefineForClassFields: false, the field initializer runs in theconstructor as
this.id = …, invoking the nativeidsetter, which adds anidattribute to the element during construction.Per the DOM "create an element" algorithm,
an already-defined custom element constructed synchronously
(
document.createElement/createElementNS) must be pristine — no attributes,no children — or the browser throws
NotSupportedError: The result must not have attributes(HTML custom-element requirement).
Frameworks that construct elements eagerly hit this. Vue's renderer calls
document.createElement('limel-checkbox')on the already-registered tag, so itupgrades synchronously, the constructor adds
id, and it throws — the checkboxnever hydrates (no shadow root, no
hydratedclass). Sibling components(
limel-button,limel-input-field, …) don't assignthis.id, so they'reunaffected. The Stencil lazy-load / HTML-parser path never trips the check,
which is why this wasn't caught before.
Fix
Rename the field to
inputId(consistent with the siblinghelperTextId) so itno longer shadows the native property, and update its read in
render().checkbox.template.tsxis unchanged — itsidprop is a key on a plain propsobject, not on an
HTMLElement.No behaviour change to the internal
<input>/<label>wiring. Bonus: the hostelement no longer gets a stray generated
id, and a consumer-set<limel-checkbox id="…">is now left untouched (previously the constructoroverwrote it).
Tests
Added spec coverage in
checkbox.spec.tsx:id(the regression);idis preserved;forstill matches it.Related (not in this PR)
The same
private idpattern exists indialog/dialog.tsx, but there it'sassigned in
componentWillLoad()rather than a field initializer, so it dodgesthe crash — though it still stamps a generated
idonto the host. Worth thesame rename as a follow-up.
radio-button-group/radio-button.tsxexposes adeliberate
@Prop() id(Stencil accessors shadow the native setter; noconstructor assignment) and is not affected.