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

fix #4041, warn overriding Vue's internal methods #4111

Merged
merged 2 commits into from
Nov 4, 2016

Conversation

HerringtonDarkholme
Copy link
Member

No description provided.

`method "${key}" has an undefined value in the component definition. ` +
`Did you reference the function correctly?`,
vm
)
hasOwn(BuiltinVue.prototype, key) && warn(
`You're overriding Vue's internal method "${key}". ` +
`Beware of misbehaviors.`,
Copy link
Member

Choose a reason for hiding this comment

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

Let rephrase this to "Avoid overriding Vue's internaml method ..." and can remove the second line.

@yyx990803 yyx990803 merged commit 4078ce9 into vuejs:dev Nov 4, 2016
@phanan
Copy link
Member

phanan commented Nov 5, 2016

@yyx990803 As discussed on Slack, this commit only solves half of the problem, as it doesn't cover the properties, which can be more tricky.

`method "${key}" has an undefined value in the component definition. ` +
`Did you reference the function correctly?`,
vm
)
hasOwn(BuiltinVue.prototype, key) && warn(
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just use vm instead of importing Vue here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, I also considered that approach when coding this. But if we use vm or vm.constructor.prototype, it will break constructor style instance because $emit or so isn't their own properties.

@phanan
Copy link
Member

phanan commented Nov 5, 2016

Doesn't a simple key in vm work just fine?

On Saturday, November 5, 2016, (´・ω・`) notifications@github.com wrote:

@HerringtonDarkholme commented on this pull request.

In src/core/instance/state.js #4111:

       `method "${key}" has an undefined value in the component definition. ` +
       `Did you reference the function correctly?`,
       vm
     )
  •    hasOwn(BuiltinVue.prototype, key) && warn(
    

Hi, I also considered that approach when coding this. But if we use vm or
vm.constructor.prototype, it will break constructor style instance
because $emit or so isn't their own properties.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#4111, or mute the thread
https://github.com/notifications/unsubscribe-auth/AHrt0uN6VSZW78hdY5wLzpaeLaHGzX7tks5q6-4jgaJpZM4KpNci
.

@HerringtonDarkholme
Copy link
Member Author

I'm sorry that this change introduced more problems than it resolved.
As @phanan pointed out, only method is warned against. Also #4142 is another problem caused by this.

I have reconsidered this. We want to only warn against overriding built-in method. And using key in vm to detect that effectively means:

  • vm only has properties inherited from Vue.prototype. This is guaranteed by Vue's core code, where extending is done by option merging.
  • Vue.prototype does not have user defined method. This is not controlled by core code, as Internal method overriding detection #4142 showed.

@phanan Could you tell me the tricky part of properties? I'm not on slack.

@phanan
Copy link
Member

phanan commented Nov 7, 2016

@HerringtonDarkholme: @yyx990803 should have a better answer because he knows best about the framework's internal obviously, but from what I can tell, not all properties have been registered by the time methods are collected (examples are $el and $slots). This would make a check during initMethods() incomplete and was what kept me from sending a PR over.

@HerringtonDarkholme
Copy link
Member Author

HerringtonDarkholme commented Nov 7, 2016

Yes, I just tried to fix this again and found warning is particularly tricky thing. More importantly, a _ and $ prefix have been strong indicators to users. The marginal benefit for this pr doesn't worth... @yyx990803 I'm so sorry make this rushy pull request. Feel free if you want to revert this.

@yyx990803
Copy link
Member

I'll just revert this for now - I think this is a very rare case anyway. Or, we can simply change it to warn against any method that starts with $ or _, since this is conventionally reserved for internal and plugin methods/properties.

@phanan
Copy link
Member

phanan commented Nov 7, 2016

Agreed. This "improvement" has only been costing us time with no particular benefits to start with :)

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.

None yet

3 participants