Warn and handle missing get in computed (fix #5265) #5267
Conversation
src/core/instance/state.js
Outdated
| let getter = typeof userDef === 'function' ? userDef : userDef.get | ||
| if (getter === undefined) { | ||
| warn( | ||
| `computed "${key}" is requires a "get" to be defined if a "set" is defined.` + |
There was a problem hiding this comment.
What about saying: "No getter function has been defined for computed property ${key}."
src/core/instance/state.js
Outdated
| if (getter === undefined) { | ||
| warn( | ||
| `computed "${key}" is requires a "get" to be defined if a "set" is defined.` + | ||
| `Did you define or misspell the get?`, |
There was a problem hiding this comment.
I don't think we need this.
src/core/instance/state.js
Outdated
| @@ -147,7 +147,15 @@ function initComputed (vm: Component, computed: Object) { | |||
|
|
|||
| for (const key in computed) { | |||
| const userDef = computed[key] | |||
| const getter = typeof userDef === 'function' ? userDef : userDef.get | |||
| let getter = typeof userDef === 'function' ? userDef : userDef.get | |||
| if (getter === undefined) { | |||
There was a problem hiding this comment.
Can you wrap the if in a non-prod if please? (if (process.env.NODE_ENV !== 'production'))
There was a problem hiding this comment.
Should only the warning be wrapped? I think getter should be assigned to noop regardless of whether it is in production mode or not, so that the behavior is consistent.
There was a problem hiding this comment.
I don't think it's necessary to have a fallback on production because we'd get the error at runtime, so wrapping the whole if looks better imo. It'll also prevent an unnecessary check for all computed properties
8ad8be5 to
dd14280
Compare
| @@ -147,7 +147,16 @@ function initComputed (vm: Component, computed: Object) { | |||
|
|
|||
| for (const key in computed) { | |||
| const userDef = computed[key] | |||
| const getter = typeof userDef === 'function' ? userDef : userDef.get | |||
| let getter = typeof userDef === 'function' ? userDef : userDef.get | |||
| if (process.env.NODE_ENV !== 'production') { | |||
There was a problem hiding this comment.
if(process.env.NODE_ENV !== 'production' && !getter) {
// warning
}might be better
There was a problem hiding this comment.
I don't think so. Former is better as define plugin would replace process.env.NODE_ENV with 'production' and it can be stripped off.
|
Here is the change that changed the behavior. Notice on this line that This would be consistent with how IMO this is better because it would not cause errors in production mode, avoid a new warning, keep the Alternatively, I think it may be possible to avoid creating a Watcher if |
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
devbranch for v2.x (or to a previous version branch), not themasterbranchfix #xxx[,#xxx], where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information:
I ran all test locally. I'm having some failures related to SSR and selenium which appear to be completely unrelated. I've confirmed the new test works by reverting my change and adding it back in.
I am using
noopfor the getter if one does not exist to avoid exceptions when calling functions on the getter.