Warn and handle missing get in computed (fix #5265) #5267

kmc059000 · 2017-03-23T21:42:23Z

What kind of change does this PR introduce? (check at least one)

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

It's submitted to the dev branch for v2.x (or to a previous version branch), not the master branch
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
All tests are passing: https://github.com/vuejs/vue/blob/dev/.github/CONTRIBUTING.md#development-setup
New/updated tests are included

If adding a new feature, the PR's description includes:

A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

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 noop for the getter if one does not exist to avoid exceptions when calling functions on the getter.

kmc059000 · 2017-03-23T21:43:17Z

#5265

rayrutjes · 2017-03-23T21:56:26Z

src/core/instance/state.js

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


What about saying: "No getter function has been defined for computed property ${key}."

rayrutjes · 2017-03-23T21:56:56Z

src/core/instance/state.js

+    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?`,


I don't think we need this.

posva · 2017-03-23T22:00:36Z

src/core/instance/state.js

@@ -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) {


Can you wrap the if in a non-prod if please? (if (process.env.NODE_ENV !== 'production'))

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.

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

javoski · 2017-03-24T01:25:21Z

src/core/instance/state.js

@@ -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') {


if(process.env.NODE_ENV !== 'production' && !getter) { // warning }

might be better

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.

both are stripped off actually

kmc059000 · 2017-03-24T02:04:46Z

Here is the change that changed the behavior. Notice on this line that noop is used if get is falsy. Does anybody think that this original behavior is preferable to the warning?

const getter = typeof userDef === 'function' ? userDef : (userDef.get || noop)

This would be consistent with how defineComputed() is defined here when the get is undefined.

IMO this is better because it would not cause errors in production mode, avoid a new warning, keep the
2.1.* behavior, and make the behavior between production and non-production modes consistent.

Alternatively, I think it may be possible to avoid creating a Watcher if get is not defined, although I think this is a edge case.

watchers[key] = getter ? new Watcher(vm, getter, noop, computedWatcherOptions) : null

rayrutjes reviewed Mar 23, 2017

View reviewed changes

posva reviewed Mar 23, 2017

View reviewed changes

warn and handle missing get in computed (fix vuejs#5265)

dd14280

kmc059000 force-pushed the 5265-warn-computed-missing-get branch from 8ad8be5 to dd14280 Compare March 23, 2017 22:37

kazupon approved these changes Mar 24, 2017

View reviewed changes

javoski reviewed Mar 24, 2017

View reviewed changes

yyx990803 merged commit 6fcfdbd into vuejs:dev Mar 27, 2017

awamwang pushed a commit to awamwang/vue that referenced this pull request Jun 15, 2017

warn and handle missing get in computed (fix vuejs#5265) (vuejs#5267)

7a2e2b3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn and handle missing get in computed (fix #5265) #5267

Warn and handle missing get in computed (fix #5265) #5267

kmc059000 commented Mar 23, 2017 •

edited

kmc059000 commented Mar 23, 2017

rayrutjes Mar 23, 2017

rayrutjes Mar 23, 2017

posva Mar 23, 2017

kmc059000 Mar 23, 2017

posva Mar 23, 2017

javoski Mar 24, 2017

znck Mar 24, 2017

posva Mar 24, 2017 •

edited

kmc059000 commented Mar 24, 2017 •

edited

Warn and handle missing get in computed (fix #5265) #5267

Warn and handle missing get in computed (fix #5265) #5267

Conversation

kmc059000 commented Mar 23, 2017 • edited

kmc059000 commented Mar 23, 2017

rayrutjes Mar 23, 2017

Choose a reason for hiding this comment

rayrutjes Mar 23, 2017

Choose a reason for hiding this comment

posva Mar 23, 2017

Choose a reason for hiding this comment

kmc059000 Mar 23, 2017

Choose a reason for hiding this comment

posva Mar 23, 2017

Choose a reason for hiding this comment

javoski Mar 24, 2017

Choose a reason for hiding this comment

znck Mar 24, 2017

Choose a reason for hiding this comment

posva Mar 24, 2017 • edited

Choose a reason for hiding this comment

kmc059000 commented Mar 24, 2017 • edited

kmc059000 commented Mar 23, 2017 •

edited

posva Mar 24, 2017 •

edited

kmc059000 commented Mar 24, 2017 •

edited