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.

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

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

Merged
merged 12 commits into from
Apr 20, 2020

Conversation

aztalbot
Copy link
Contributor

This is meant to address #1001 by separating out onXXX attribute keys when fall-through can't be performed due to multiple root nodes. It warns that there are extra non-emits event listeners, indicating to the user that they should consider adding the events to the emits option.


This PR also adds logic to call markAttrsAccessed when rendering functional components that have either optional props (so technically there are no extra attrs) or that access context.attrs similar to how stateful components work. This prevents the warning in those cases.

Copy link
Member

@yyx990803 yyx990803 left a comment

Choose a reason for hiding this comment

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

I accidentally worked on the same thing but I think this is more comprehensive. Can you resolve the conflict please? Thanks!

`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.

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?

@yyx990803 yyx990803 merged commit cebad64 into vuejs:master Apr 20, 2020
pikax pushed a commit to pikax/vue-next that referenced this pull request Apr 29, 2020
@shortpoet
Copy link

I'm not very experienced so, not sure if it helps, or if this is the correct place, but just in case...

I came across this issue when googling the following warning:

Extraneous non-emits event listeners (update-contacts) were passed to component but could not be automatically inherited because component renders fragment or text root nodes. If the listener is intended to be a component custom event listener only, declare it using the "emits" option.

After looking here and at some articles about Vue 3, I came to the conclusion that adding

emits: ['update-contacts']

would be a good idea.

That got rid of the warnings. A small point of confusion for me was why it still worked even without the emits property. Also, why this only happened when I added a grand-child component where the middle component just basically passed along the emitted event (no payload).

I don't expect to receive explanations here. Just hoping to do my part and provide useful (hopefully) feedback. Please let me know if I should refrain or direct them elsewhere.

Thanks again for the hard work on a great framework!

@softwareCobbler
Copy link

Hello, I think this error message could be improved to also say something like "you may not have intended to have this listener fallthrough; consider inheritAttrs: false"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants