Skip to content

improve warning when extra event listeners (fix #1001) #1005

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.

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

Merged
merged 12 commits into from
Apr 20, 2020
72 changes: 69 additions & 3 deletions packages/runtime-core/__tests__/rendererAttrsFallthrough.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ describe('attribute fallthrough', () => {
it('should warn when fallthrough fails on non-single-root', () => {
const Parent = {
render() {
return h(Child, { foo: 1, class: 'parent' })
return h(Child, { foo: 1, class: 'parent', onBar: () => {} })
}
}

Expand All @@ -353,12 +353,13 @@ describe('attribute fallthrough', () => {
render(h(Parent), root)

expect(`Extraneous non-props attributes (class)`).toHaveBeenWarned()
expect(`Extraneous non-emits event listeners`).toHaveBeenWarned()
})

it('should not warn when $attrs is used during render', () => {
const Parent = {
render() {
return h(Child, { foo: 1, class: 'parent' })
return h(Child, { foo: 1, class: 'parent', onBar: () => {} })
}
}

Expand All @@ -374,13 +375,15 @@ describe('attribute fallthrough', () => {
render(h(Parent), root)

expect(`Extraneous non-props attributes`).not.toHaveBeenWarned()
expect(`Extraneous non-emits event listeners`).not.toHaveBeenWarned()

expect(root.innerHTML).toBe(`<div></div><div class="parent"></div>`)
})

it('should not warn when context.attrs is used during render', () => {
const Parent = {
render() {
return h(Child, { foo: 1, class: 'parent' })
return h(Child, { foo: 1, class: 'parent', onBar: () => {} })
}
}

Expand All @@ -396,9 +399,72 @@ describe('attribute fallthrough', () => {
render(h(Parent), root)

expect(`Extraneous non-props attributes`).not.toHaveBeenWarned()
expect(`Extraneous non-emits event listeners`).not.toHaveBeenWarned()

expect(root.innerHTML).toBe(`<div></div><div class="parent"></div>`)
})

it('should not warn when context.attrs is used during render (functional)', () => {
const Parent = {
render() {
return h(Child, { foo: 1, class: 'parent', onBar: () => {} })
}
}

const Child: FunctionalComponent = (_, { attrs }) => [
h('div'),
h('div', attrs)
]

Child.props = ['foo']

const root = document.createElement('div')
document.body.appendChild(root)
render(h(Parent), root)

expect(`Extraneous non-props attributes`).not.toHaveBeenWarned()
expect(`Extraneous non-emits event listeners`).not.toHaveBeenWarned()
expect(root.innerHTML).toBe(`<div></div><div class="parent"></div>`)
})

it('should not warn when functional component has optional props', () => {
const Parent = {
render() {
return h(Child, { foo: 1, class: 'parent', onBar: () => {} })
}
}

const Child = (props: any) => [h('div'), h('div', { class: props.class })]

const root = document.createElement('div')
document.body.appendChild(root)
render(h(Parent), root)

expect(`Extraneous non-props attributes`).not.toHaveBeenWarned()
expect(`Extraneous non-emits event listeners`).not.toHaveBeenWarned()
expect(root.innerHTML).toBe(`<div></div><div class="parent"></div>`)
})

it('should warn when functional component has props and does not use attrs', () => {
const Parent = {
render() {
return h(Child, { foo: 1, class: 'parent', onBar: () => {} })
}
}

const Child: FunctionalComponent = () => [h('div'), h('div')]

Child.props = ['foo']

const root = document.createElement('div')
document.body.appendChild(root)
render(h(Parent), root)

expect(`Extraneous non-props attributes`).toHaveBeenWarned()
expect(`Extraneous non-emits event listeners`).toHaveBeenWarned()
expect(root.innerHTML).toBe(`<div></div><div></div>`)
})

// #677
it('should update merged dynamic attrs on optimized child root', async () => {
const aria = ref('true')
Expand Down
4 changes: 3 additions & 1 deletion packages/runtime-core/src/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,9 @@ function finishComponentSetup(

const attrHandlers: ProxyHandler<Data> = {
get: (target, key: string) => {
markAttrsAccessed()
if (__DEV__) {
markAttrsAccessed()
}
return target[key]
},
set: () => {
Expand Down
45 changes: 38 additions & 7 deletions packages/runtime-core/src/componentRenderUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,19 @@ export function renderComponentRoot(
} else {
// functional
const render = Component as FunctionalComponent
// in dev, mark attrs accessed if optional props (attrs === props)
if (__DEV__ && attrs === props) {
markAttrsAccessed()
}
result = normalizeVNode(
render.length > 1
? render(props, {
attrs,
get attrs() {
Copy link
Member

Choose a reason for hiding this comment

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

Should only make attrs a getter in dev.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Is it okay, though, to use __DEV__ with a ternary to achieve that?

if (__DEV__ && attrs !== props) {
markAttrsAccessed()
}
return attrs
},
slots,
emit
})
Expand Down Expand Up @@ -107,12 +116,34 @@ export function renderComponentRoot(
root.patchFlag |= PatchFlags.FULL_PROPS
}
} else if (__DEV__ && !accessedAttrs && root.type !== Comment) {
warn(
`Extraneous non-props attributes (` +
`${Object.keys(attrs).join(', ')}) ` +
`were passed to component but could not be automatically inherited ` +
`because component renders fragment or text root nodes.`
)
const allAttrs = Object.keys(attrs)
const eventAttrs: string[] = []
const extraAttrs: string[] = []
for (let i = 0, l = allAttrs.length; i < l; i++) {
const key = allAttrs[i]
if (isOn(key)) {
// remove `on`, lowercase first letter to reflect event casing accurately
eventAttrs.push(key[2].toLowerCase() + key.slice(3))
} else {
extraAttrs.push(key)
}
}
if (extraAttrs.length) {
warn(
`Extraneous non-props attributes (` +
`${extraAttrs.join(', ')}) ` +
`were passed to component but could not be automatically inherited ` +
`because component renders fragment or text root nodes.`
)
}
if (eventAttrs.length) {
warn(
`Extraneous non-emits event listeners (` +
`${eventAttrs.join(', ')}) ` +
`were passed to component but could not be automatically inherited ` +
`because component renders fragment or text root nodes.`
Copy link
Member

Choose a reason for hiding this comment

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

Let's add if the listener is intended to be a component custom event listener only, declare it using the "emits" option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

)
}
}
}

Expand Down