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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Apr 27, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f59c551
refactor: updated set data method and add a new test for data obj
shortdiv Apr 26, 2018
12ac952
fix: update code with suggested edits
shortdiv Apr 27, 2018
36e2366
fix: update deep merge to use lodash merge and updated tests
shortdiv Apr 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/test-utils/src/wrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,14 @@ export default class Wrapper implements BaseWrapper {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this line should be removed.

this.vm.$set(this.vm, [key], data[key])
if (typeof data[key] === typeof {} && data[key] !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Object.keys(data[key]).forEach((key2) => {
var newObj = Object.assign({}, data[key], data[key][key2])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]}

this.vm.$set(this.vm, [key], newObj)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})
} else {
this.vm.$set(this.vm, [key], data[key])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
})
}

Expand Down
21 changes: 21 additions & 0 deletions test/specs/wrapper/setData.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const Cmp = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

data: () => ({
anObject: {
propA: {
prop1: 'a'
},
propB: 'b'
}
})
}
const wrapper = mountingMethod(Cmp)
wrapper.setData({
anObject: {
propA: {
prop1: 'c'
}
}
})
expect(wrapper.vm.anObject.propA.prop1).to.equal('c')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})
})