Skip to content

feat(compiler-dom): support customizable select #13616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −3
Open

feat(compiler-dom): support customizable select #13616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/compiler-dom/__tests__/transforms/validateHtmlNesting.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ describe('validate html nesting', () => {
expect(err!.message).toMatch(`<div> cannot be child of <p>`)
})

it("Don't warn with customize select and option", () => {
let err: CompilerError | undefined
compile(`<select><option></option></select>`, { onWarn: e => (err = e) })
compile(`<option><p></p></option>`, { onWarn: e => (err = e) })
expect(err).toBeUndefined()
})

Comment on lines +14 to +20
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix test logic and improve test structure.

The test has a potential issue where the err variable is shared between two compile calls, which could lead to false test results. If the first compile call generates a warning but the second doesn't, the test would incorrectly pass.

Additionally, the test name has grammatical issues and should be corrected.

Apply this diff to fix the test logic and improve the structure:

-  it("Don't warn with customize select and option", () => {
+  it("should not warn with customizable select and option", () => {
     let err: CompilerError | undefined
     compile(`<select><option></option></select>`, { onWarn: e => (err = e) })
+    expect(err).toBeUndefined()
+    
+    err = undefined // Reset for next test
     compile(`<option><p></p></option>`, { onWarn: e => (err = e) })
     expect(err).toBeUndefined()
   })

Alternatively, consider splitting this into two separate test cases for better clarity and isolation:

-  it("Don't warn with customize select and option", () => {
-    let err: CompilerError | undefined
-    compile(`<select><option></option></select>`, { onWarn: e => (err = e) })
-    compile(`<option><p></p></option>`, { onWarn: e => (err = e) })
-    expect(err).toBeUndefined()
-  })
+  it('should not warn with customizable select', () => {
+    let err: CompilerError | undefined
+    compile(`<select><option></option></select>`, { onWarn: e => (err = e) })
+    expect(err).toBeUndefined()
+  })
+
+  it('should not warn with customizable option', () => {
+    let err: CompilerError | undefined
+    compile(`<option><p></p></option>`, { onWarn: e => (err = e) })
+    expect(err).toBeUndefined()
+  })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("Don't warn with customize select and option", () => {
let err: CompilerError | undefined
compile(`<select><option></option></select>`, { onWarn: e => (err = e) })
compile(`<option><p></p></option>`, { onWarn: e => (err = e) })
expect(err).toBeUndefined()
})
it("should not warn with customizable select and option", () => {
let err: CompilerError | undefined
compile(`<select><option></option></select>`, { onWarn: e => (err = e) })
expect(err).toBeUndefined()
err = undefined // Reset for next test
compile(`<option><p></p></option>`, { onWarn: e => (err = e) })
expect(err).toBeUndefined()
})
🤖 Prompt for AI Agents 
In packages/compiler-dom/__tests__/transforms/validateHtmlNesting.spec.ts around
lines 14 to 20, the test shares the same error variable between two compile
calls, which can cause false positives if the first call sets an error but the
second does not. To fix this, separate the tests into two distinct test cases
each with its own error variable to isolate warnings properly. Also, rename the
test to correct the grammar for clarity, for example, change "Don't warn with
customize select and option" to "Does not warn with customized select and
option."

it('should not warn with select > hr', () => {
let err: CompilerError | undefined
compile(`<select><hr></select>`, {
Expand Down
3 changes: 0 additions & 3 deletions packages/compiler-dom/src/htmlNesting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ const onlyValidChildren: Record<string, Set<string>> = {
'script',
'template',
]),
optgroup: new Set(['option']),
select: new Set(['optgroup', 'option', 'hr']),
// table
table: new Set(['caption', 'colgroup', 'tbody', 'tfoot', 'thead']),
tr: new Set(['td', 'th']),
Expand All @@ -74,7 +72,6 @@ const onlyValidChildren: Record<string, Set<string>> = {
// these elements can not have any children elements
script: emptySet,
iframe: emptySet,
option: emptySet,
textarea: emptySet,
style: emptySet,
title: emptySet,
Expand Down