method named _update breaks component creation

[JustinBeaudry]

Version

2.4.2

Reproduction link

[Method]
https://jsfiddle.net/50wL7mdz/52180/
and
[Computed]
https://jsfiddle.net/7d2aytsL/

Steps to reproduce

1. Add an _update method or computed property on a custom component
2. Add that component to the App
3. Note Vue throws TypeError

What is expected?

An method called _update is a perfectly reasonable name for a method and/or computed property, and should not be throwing simply based on it's naming.

What is actually happening?

I have yet to dig in further but for some reason the vnode object loses it's reference when having this method or computed name.

Uncaught TypeError: Cannot read property 'componentInstance' of null
    at isPatchable ([email protected]:5217)
    at initComponent ([email protected]:5160)
    at createComponent ([email protected]:5145)
    at createElm ([email protected]:5081)
    at createChildren ([email protected]:5209)
    at createElm ([email protected]:5114)
    at Vue$3.patch [as __patch__] ([email protected]:5597)
    at Vue$3.Vue._update ([email protected]:2418)
    at Vue$3.updateComponent ([email protected]:2542)
    at Watcher.get ([email protected]:2883)

[LinusBorg]

Vue itself uses underscore prefixes for internal methods. It's not advised you do so.

[posva]

PRs to add warnings that check the methods names are welcome I guess but, yeah, don't name methods with a starting underscore

[JustinBeaudry]

I didn't note that in the docs. I think it's unreasonable to expect people to have to respect naming strategies for methods, however, I understand it may not be so simple to make that change. I'll make a PR to add some warnings, and to make some notes in the documentation.

[HerringtonDarkholme]

@posva I believe similar PRs has existed before to add warning, but it turned out adding warning isn't easy.

Vue adds methods one by one so it is hard to tell which word is internal.

[posva]

Damn, I didn't think it will be hard 🙁
In that case, it's worth pointing it out in the docs
In future versions we can use Symbols to prevent this, right?

[JustinBeaudry]

@posva I'm still unfamiliar with the Vue codebase, but Symbols seem like a great way to mitigate this issue entirely. The other idea, though it's a breaking change, is to simply namespace the methods into this.methods or something like that.

As for updating the docs. PR here: vuejs/v2.vuejs.org#1072

I didn't really see a good place to add a note about naming methods in the guide, perhaps that deserves it's own heading, but I'm still trying to understand the codebase so I'd rather not write a guide to something I don't completely understand.

[chrisvfritz]

@posva @HerringtonDarkholme @JustinBeaudry I might be misunderstanding, but would it not be possible to provide a warning here, if key[0] === '_' and perhaps also if key[0] === '$'?

[JustinBeaudry]

@chrisvfritz I was trying something like that locally this morning to see if that would work and it seems to. Instead of trying to warn against collisions against specific names, just warning about method names with _ or $ seems to be the best bet for now. A warning almost doesn't seem harsh enough though IMO.

[HerringtonDarkholme]

@chrisvfritz The problem is some library will use prefixed method names to hide implementation.
If we warn based on prefix, library users will have undesired warning.

[JustinBeaudry]

@HerringtonDarkholme But prefixed names shouldn't be used, due to the possible namespace collisions. It seems unreasonable to have to find every internal method used and only warn when a method conflicts with that. IMO It's either warn on every prefixed method name (it is just a warning after all) or namespace methods.

[chrisvfritz]

@HerringtonDarkholme I agree with @JustinBeaudry in this case. It sounds like we all suggest not to use any underscore-prefixed datum/prop/computed/method names, because we can't guarantee that Vue won't conflict with them in the future. That means these plugins could already break at any moment, including in a patch release.

I think it'd be better to have a one-time "soft break" with a warning that suggests an alternate strategy. Maybe the $_ prefix could be officially reserved for private plugin properties, with a suggested namespace like $_pluginName_methodName to avoid conflicts with other plugins. I use a similar strategy for even application-specific plugins/mixins.

Then for any property registration, we could just display a warning if /^(\$[^_]|_)/.test(key).

[chrisvfritz]

As a slight aside for private methods specifically, taking advantage of JS scoping might make more sense. For example:

export default {
  ...
}

function myPrivateFunc (vm) { ... }

[HerringtonDarkholme]

@chrisvfritz Agree. Introducing a new naming pattern is good enough before we can safely use Symbol.

[posva]

IMO It's either warn on every prefixed method name (it is just a warning after all) or namespace methods.

We cannot say it's just a warning. Introducing a new naming method convention will make a lot of library creators have warnings and fill the console for people using those plugins, making it very difficult to debug. Furthermore, more people will come and complain and create issues for nothing not only on Vue's repository but also on plugins'

Even if Vue add methods one by one, we can create a static list of methods names that shouldn't be used, can't we?

[chrisvfritz]

@posva

Even if Vue add methods one by one, we can create a static list of methods names that shouldn't be used, can't we?

Unfortunately, we can't create a static list of all private property names that might be used in the future. And maintaining that list would be a pain, easily falling out of sync.

Introducing a new naming method convention will make a lot of library creators have warnings and fill the console for people using those plugins, making it very difficult to debug.

Not necessarily. We could start by introducing a new config (e.g. Vue.config.compatErrors = true), which when enabled, could throw actual errors with stack traces so you can see exactly where the method was defined. When we release it, we could reach out to the biggest plugin authors personally, suggesting that they enable it by default in their tests.

Once plugin authors have been trained, we could include a softer warning by default, e.g. with:

console.warn(new Error(...))

That way:

• Anyone still affected will see warnings instead of scarier errors
• Apps won't break (even in dev), unless they were already broken due to a real conflict
• The warnings will still include stack traces, so that users can see exactly where the problem came from, if it's not obvious from the name
• The message could even describe that it's possible a plugin is causing this naming conflict. Most people will probably not be using so many plugins that they can't narrow it down, even if they're not good at reading stack traces. 🙂

In the future, Vue.config.compatErrors could continue to be used for other compatibility conventions we want to establish, such as if we decided to warn about component names without a dash when using the standalone build. (Not a suggestion I'm making right now btw. 😄 )

Thoughts?

[posva]

It looks like a huge amount of work for very little benefit 😰
I think it'll be easier to answer to every issue that may arise in the future (which will be very little) than contacting every big plugin author

From my pov, a note on the docs is the best option. When using an _ or $ in front of a property, the user must be aware of what he's doing, so recommending using $_ looks like a good trade off

[JustinBeaudry]

I have reproduced this with the computed property as well. :-( I've updated the issue.

[chrisvfritz]

@posva I agree that right now, the problem is relatively small - at least as far as we can measure, since overriding private properties can produce difficult-to-diagnose errors, or worse, subtle bugs that don't even produce an error. Since it's difficult to track down and report, it might remain a largely invisible problem.

And the longer we wait on something like this, the more plugins will exist and the more conflicts will arise, both with Vue and between plugins/mixins. I feel like we can wait until it's a bigger problem, but it's guaranteed to keep growing with Vue's popularity and it'll just be even more work at that point.

---

@JustinBeaudry Yes, we'll have to warn about all top-level properties: data, props, computed properties, and methods.

[posva]

I still think that a more pragmatic way will be to document it because if people have built plugins that work, they've probably got around that naming restriction already. I really think we're trying to solve a problem that doesn't exist. What may happen in the future is that new users trying to create plugins face the issue and find out about this thread and end up fixing it in a couple of hours max

[chrisvfritz]

@posva It sounds like your primary concern is just whether it's worth the effort. Is that accurate? If so, I think most of the work towards an ESLint rule is already done and I wouldn't mind writing the optional warnings behind the config flag. As a dev-only feature, it also wouldn't add to the weight of the production build. Does that sound acceptable, as a place to start?

[posva]

@chrisvfritz No, no, that's not my main concern 😆
I think we have to be careful about adding warnings because it does impact development experience for everybody while it may help only a dozen of persons. That's why I sometimes think adding more documentation and eslint plugins are better

[chrisvfritz]

@posva What impact on the development experience are you trying to avoid? If it's behind a config flag, wouldn't it only affect the experience for those who care about future compatibility? And always only in a positive way? I think hiding this important warning behind a compatibility flag is more likely to make people angry than offering them a chance to take a few minutes to prevent their apps from breaking in the future. 😄

I see this as similar to many security issues. We use SSL, even though a user's credentials probably won't be stolen. We're eliminating an especially ugly scenario, despite a relatively low likelihood of occurrence. And in both cases, it's an easy fix that you only have to make once.

If an entire Vue app could break at any time, in a way that would be difficult to diagnose or possibly even detect, do you not feel that's worth a couple minutes to fix, even if it probably won't happen?

[posva]

By development experience, I also mean that most of the time it has an impact on performance. That wasn't clear, sorry. But this really depends on when does the check happen.
Having a flag to deactivate the tip/warning looks reasonable

[chrisvfritz]

Ah, I see. Since the check only happens on initialization and we're already iterating over each property for other checks, the impact should be minimal.