refactor: Update setData method (address #563) #565

shortdiv · 2018-04-26T19:34:43Z

This is a refactor of the setData method to allow for setting data values and objects. The current implementation of setData only allows for setting values and not objects.

Addresses #563

eddyerburgh · 2018-04-26T20:17:45Z

Ah I'm so sorry, I wasn't clear in my comment!

I think we should keep the signature the same as it is currently. Instead, we could update the function to do a deep merge:

const Cmp = {
  data: () => ({
     anObject: {
       propA: 'a',
       propB: 'b'
     }
  }) 
}
const wrapper = mount(Cmp)
wrapper.setData({
  anObject: {
    propB: 'c'
  }
})
wrapper.vm.anObject.propA // 'a'
wrapper.vm.anObject.propB // 'c'

shortdiv · 2018-04-26T20:18:48Z

Ah gotcha, will update accordingly!

38elements · 2018-04-27T04:06:55Z

packages/test-utils/src/wrapper.js

@@ -421,7 +421,14 @@ export default class Wrapper implements BaseWrapper {

    Object.keys(data).forEach((key) => {
      // $FlowIgnore : Problem with possibly null this.vm


I think that this line should be removed.

38elements · 2018-04-27T04:11:40Z

packages/test-utils/src/wrapper.js

+      if (typeof data[key] === typeof {} && data[key] !== null) {
+        Object.keys(data[key]).forEach((key2) => {
+          var newObj = Object.assign({}, data[key], data[key][key2])
+          this.vm.$set(this.vm, [key], newObj)


I think it is necessary to add the following line before this line.
// $FlowIgnore : Problem with possibly null this.vm

38elements · 2018-04-27T04:12:14Z

packages/test-utils/src/wrapper.js

+          this.vm.$set(this.vm, [key], newObj)
+        })
+      } else {
+        this.vm.$set(this.vm, [key], data[key])


I think it is necessary to add the following line before this line.
// $FlowIgnore : Problem with possibly null this.vm

38elements · 2018-04-27T04:21:46Z

packages/test-utils/src/wrapper.js

-      this.vm.$set(this.vm, [key], data[key])
+      if (typeof data[key] === typeof {} && data[key] !== null) {
+        Object.keys(data[key]).forEach((key2) => {
+          var newObj = Object.assign({}, data[key], data[key][key2])


I think spread syntax is frequently used in this repository.
I think it is better to change var to const.

const newObj = { ...data[key], ...data[key][key2]}

38elements · 2018-04-27T04:29:29Z

packages/test-utils/src/wrapper.js

@@ -421,7 +421,14 @@ export default class Wrapper implements BaseWrapper {

    Object.keys(data).forEach((key) => {
      // $FlowIgnore : Problem with possibly null this.vm
-      this.vm.$set(this.vm, [key], data[key])
+      if (typeof data[key] === typeof {} && data[key] !== null) {


Would you tell me why are you using typeof data[key] === typeof {}, not typeof data[key] === 'object' ?
I think typeof data[key] === 'object' is better.

The null check was necessary because null is of type object. If you're updating an object with {message: null}, it will pass this conditional, which it technically shouldn't since it's not a nested object.

38elements · 2018-04-27T04:31:54Z

It is necessary to execute yarn flow.

38elements · 2018-04-27T04:44:30Z

packages/test-utils/src/wrapper.js

        Object.keys(data[key]).forEach((key2) => {
-          var newObj = Object.assign({}, data[key], data[key][key2])
+          const newObj = { ...data[key], ...data[key][key2]}


It is necessary to add space.

const newObj = { ...data[key], ...data[key][key2] }

38elements · 2018-04-27T04:45:05Z

It is necessary to execute yarn lint.

38elements · 2018-04-27T06:06:52Z

test/specs/wrapper/setData.spec.js

@@ -132,4 +132,25 @@ describeWithShallowAndMount('setData', (mountingMethod) => {
    wrapper.setData({ message: null })
    expect(wrapper.text()).to.equal('There is no message yet')
  })
+  it('should update a data object', () => {


I think it is better to add a blank line before this line.

eddyerburgh · 2018-04-27T07:01:34Z

test/specs/wrapper/setData.spec.js

+        }
+      }
+    })
+    expect(wrapper.vm.anObject.propA.prop1).to.equal('c')


Can you give propA another property, and add an assertion that the value is unchanged after calling setData.

eddyerburgh · 2018-04-27T07:01:56Z

test/specs/wrapper/setData.spec.js

@@ -132,4 +132,25 @@ describeWithShallowAndMount('setData', (mountingMethod) => {
    wrapper.setData({ message: null })
    expect(wrapper.text()).to.equal('There is no message yet')
  })
+  it('should update a data object', () => {
+    const Cmp = {


The convention we follow in this repo is to name these TestComponent

shortdiv · 2018-04-27T16:00:26Z

Updated! ✨

eddyerburgh

Great work, thanks :)

shortdiv force-pushed the dev branch 2 times, most recently from c3874ae to 5b24600 Compare April 26, 2018 20:08

shortdiv force-pushed the dev branch 2 times, most recently from 59a26a0 to 37924d7 Compare April 26, 2018 22:33

refactor: updated set data method and add a new test for data obj

f59c551

shortdiv force-pushed the dev branch from 37924d7 to f59c551 Compare April 27, 2018 02:57

38elements reviewed Apr 27, 2018

View reviewed changes

fix: update code with suggested edits

12ac952

shortdiv force-pushed the dev branch from 0c31091 to 12ac952 Compare April 27, 2018 04:45

38elements reviewed Apr 27, 2018

View reviewed changes

eddyerburgh reviewed Apr 27, 2018

View reviewed changes

fix: update deep merge to use lodash merge and updated tests

36e2366

shortdiv force-pushed the dev branch from 618641f to 36e2366 Compare April 27, 2018 15:40

eddyerburgh approved these changes Apr 27, 2018

View reviewed changes

eddyerburgh merged commit 878bccd into vuejs:dev Apr 27, 2018

johnyluyte mentioned this pull request May 14, 2018

Adding method to set data object #563

Closed

lchanmann mentioned this pull request May 14, 2018

fix: Do not deep merge array data in setData #604

Merged

Mardoxx mentioned this pull request Jul 3, 2018

Computed property not updating when based on new state object property and used by template #798

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Update setData method (address #563) #565

refactor: Update setData method (address #563) #565

shortdiv commented Apr 26, 2018

eddyerburgh commented Apr 26, 2018 •

edited

shortdiv commented Apr 26, 2018

38elements Apr 27, 2018

38elements Apr 27, 2018

38elements Apr 27, 2018

38elements Apr 27, 2018

38elements Apr 27, 2018 •

edited

shortdiv Apr 27, 2018

38elements commented Apr 27, 2018

38elements Apr 27, 2018

38elements commented Apr 27, 2018

38elements Apr 27, 2018

eddyerburgh Apr 27, 2018

eddyerburgh Apr 27, 2018

shortdiv commented Apr 27, 2018

eddyerburgh left a comment

		@@ -421,7 +421,14 @@ export default class Wrapper implements BaseWrapper {

		Object.keys(data).forEach((key) => {
		// $FlowIgnore : Problem with possibly null this.vm

refactor: Update setData method (address #563) #565

refactor: Update setData method (address #563) #565

Conversation

shortdiv commented Apr 26, 2018

eddyerburgh commented Apr 26, 2018 • edited

shortdiv commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

38elements Apr 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

38elements commented Apr 27, 2018

Choose a reason for hiding this comment

38elements commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shortdiv commented Apr 27, 2018

eddyerburgh left a comment

Choose a reason for hiding this comment

eddyerburgh commented Apr 26, 2018 •

edited

38elements Apr 27, 2018 •

edited