Skip to content
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

Jump to bottom

fix(custom-element): boolean type error in customElement #9698

Closed
wants to merge 3 commits into from
Closed

fix(custom-element): boolean type error in customElement #9698

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d49c4e
fix: boolean type error in customElement
Alfred-Skyblue Nov 28, 2023
b8eb6ea
chore: rollback lock-file
Alfred-Skyblue Nov 28, 2023
42adf11
types: declare attribute type
Alfred-Skyblue Nov 29, 2023
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
8 changes: 5 additions & 3 deletions packages/runtime-dom/__tests__/customElement.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,17 +172,19 @@ describe('defineCustomElement', () => {

e.setAttribute('bar', '')
await nextTick()
expect(e.shadowRoot!.innerHTML).toBe(`1 number true boolean 12345 string`)
Copy link
Member Author

Choose a reason for hiding this comment

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

In the original test cases, converting '' to true, I'm not sure if this aligns with expectations, but from a boolean perspective, it should be converted to false.

Copy link

Choose a reason for hiding this comment

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

HTML Living Spec says that an attribute like bar="" must be interpreted as true. Changing this to false would violate the spec and break exsiting component implementations that use defineCustomElement.

A number of attributes are boolean attributes. The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value.

If the attribute is present, its value must either be the empty string or a value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.

The values "true" and "false" are not allowed on boolean attributes. To represent a false value, the attribute has to be omitted altogether.

https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attributes

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a debate over whether, in the absence of a value or when the value is the string 'false,' it should perhaps be considered as false, while in all other cases, it should be considered as true. I have observed that performing such a conversion may result in getAttribute returning different values. It is uncertain whether this approach is reasonable, or if we should adhere to the format of Vue component attributes. However, it is worth noting that we have indeed defined a CustomElement.

expect(e.shadowRoot!.innerHTML).toBe(
`1 number false boolean 12345 string`
)

e.setAttribute('foo-bar', '2e1')
await nextTick()
expect(e.shadowRoot!.innerHTML).toBe(
`20 number true boolean 12345 string`
`20 number false boolean 12345 string`
)

e.setAttribute('baz', '2e1')
await nextTick()
expect(e.shadowRoot!.innerHTML).toBe(`20 number true boolean 2e1 string`)
expect(e.shadowRoot!.innerHTML).toBe(`20 number false boolean 2e1 string`)
})

// #4772
Expand Down
25 changes: 22 additions & 3 deletions packages/runtime-dom/src/apiCustomElement.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,14 @@ import {
SlotsType,
DefineComponent
} from '@vue/runtime-core'
import { camelize, extend, hyphenate, isArray, toNumber } from '@vue/shared'
import {
camelize,
extend,
hyphenate,
isArray,
toBoolean,
toNumber
} from '@vue/shared'
import { hydrate, render } from '.'

export type VueElementConstructor<P = {}> = {
Expand Down Expand Up @@ -177,6 +184,7 @@ export class VueElement extends BaseClass {
private _connected = false
private _resolved = false
private _numberProps: Record<string, true> | null = null
private _booleanProps: Record<string, true> | null = null
private _styles?: HTMLStyleElement[]
private _ob?: MutationObserver | null = null
constructor(
Expand Down Expand Up @@ -250,8 +258,9 @@ export class VueElement extends BaseClass {
const resolve = (def: InnerComponentDef, isAsync = false) => {
const { props, styles } = def

// cast Number-type props set before resolve
// cast Number-type and Boolean-type props set before resolve
let numberProps
let booleanProps
if (props && !isArray(props)) {
for (const key in props) {
const opt = props[key]
Expand All @@ -262,10 +271,18 @@ export class VueElement extends BaseClass {
;(numberProps || (numberProps = Object.create(null)))[
camelize(key)
] = true
} else if (opt === Boolean || (opt && opt.type === Boolean)) {
if (key in this._props) {
this._props[key] = toBoolean(this._props[key])
}
;(booleanProps || (booleanProps = Object.create(null)))[
camelize(key)
] = true
}
}
}
this._numberProps = numberProps
this._booleanProps = booleanProps

if (isAsync) {
// defining getter/setters on prototype
Expand Down Expand Up @@ -313,10 +330,12 @@ export class VueElement extends BaseClass {
}

protected _setAttr(key: string) {
let value = this.getAttribute(key)
let value = this.getAttribute(key) as any
Alfred-Skyblue marked this conversation as resolved.
Show resolved Hide resolved
const camelKey = camelize(key)
if (this._numberProps && this._numberProps[camelKey]) {
value = toNumber(value)
} else if (this._booleanProps && this._booleanProps[camelKey]) {
value = toBoolean(value)
}
this._setProp(camelKey, value, false)
}
Expand Down
14 changes: 14 additions & 0 deletions packages/shared/src/general.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,20 @@ export const toNumber = (val: any): any => {
return isNaN(n) ? val : n
}

/**
* Converts a value to a boolean. If the input value is the string 'false', the result will be `false`. Otherwise, the result will be the boolean representation of the input value using the double negation (`!!`) to coerce the value to a boolean.
*
* @example
* toBoolean('false') // false
*
* toBoolean(1) // true
*
* toBoolean(null) // false
*/
export const toBoolean = (val: any): boolean => {
return val === 'false' ? false : !!val
}

let _globalThis: any
export const getGlobalThis = (): any => {
return (
Expand Down