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

method named _update breaks component creation #6312

Closed
JustinBeaudry opened this issue Aug 7, 2017 · 24 comments
Closed

method named _update breaks component creation #6312

JustinBeaudry opened this issue Aug 7, 2017 · 24 comments

Comments

@JustinBeaudry
Copy link

JustinBeaudry commented Aug 7, 2017

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 (vue@2.4.2:5217)
    at initComponent (vue@2.4.2:5160)
    at createComponent (vue@2.4.2:5145)
    at createElm (vue@2.4.2:5081)
    at createChildren (vue@2.4.2:5209)
    at createElm (vue@2.4.2:5114)
    at Vue$3.patch [as __patch__] (vue@2.4.2:5597)
    at Vue$3.Vue._update (vue@2.4.2:2418)
    at Vue$3.updateComponent (vue@2.4.2:2542)
    at Watcher.get (vue@2.4.2:2883)
@JustinBeaudry JustinBeaudry changed the title method named _update breaks component rendering method named _update breaks component creation Aug 7, 2017
@LinusBorg
Copy link
Member

LinusBorg commented Aug 7, 2017

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

@posva
Copy link
Member

posva commented Aug 7, 2017

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

@JustinBeaudry
Copy link
Author

JustinBeaudry commented Aug 7, 2017

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
Copy link
Member

HerringtonDarkholme commented Aug 8, 2017

@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
Copy link
Member

posva commented Aug 8, 2017

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
Copy link
Author

@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
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Member

@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
Copy link
Author

@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
Copy link
Contributor

chrisvfritz commented Aug 9, 2017

@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
Copy link
Contributor

chrisvfritz commented Aug 9, 2017

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
Copy link
Member

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

@posva
Copy link
Member

posva commented Aug 9, 2017

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
Copy link
Contributor

chrisvfritz commented Aug 9, 2017

@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
Copy link
Member

posva commented Aug 9, 2017

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
Copy link
Author

JustinBeaudry commented Aug 9, 2017

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

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Aug 9, 2017

@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
Copy link
Member

posva commented Aug 9, 2017

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
Copy link
Contributor

chrisvfritz commented Aug 9, 2017

@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
Copy link
Member

posva commented Aug 15, 2017

@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
Copy link
Contributor

@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
Copy link
Member

posva commented Aug 15, 2017

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
Copy link
Contributor

chrisvfritz commented Aug 15, 2017

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.

ztlevi pushed a commit to ztlevi/vue that referenced this issue Feb 14, 2018
f2009 pushed a commit to f2009/vue that referenced this issue Jan 25, 2019
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

No branches or pull requests

5 participants