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

Fix applySnapshot to map<string,boolean> bug #155

Merged
merged 3 commits into from
May 29, 2017
Merged

Fix applySnapshot to map<string,boolean> bug #155

merged 3 commits into from
May 29, 2017

Conversation

pioh
Copy link
Contributor

@pioh pioh commented May 29, 2017

Primitive types removes from the map on applySnapshot if key exists.

const Store = types.model('Store', {
  tags: types.map(types.boolean),
})
let store = Store.create({tags: {admin: true}})
console.log(JSON.stringify(getSnapshot(store)))
applySnapshot(store, {tags: {admin: true}})
console.log(JSON.stringify(getSnapshot(store)))
applySnapshot(store, {tags: {admin: true}})
console.log(JSON.stringify(getSnapshot(store)))

Output:
{"tags":{"admin":true}}
{"tags":{}}
{"tags":{"admin":true}}

@pioh
Fix applySnapshot to map<string,boolean> bug
a12f0d6 
Primitive types removes from the map on applySnapshot if key exists.

const Store = types.model('Store', {
  tags: types.map(types.boolean),
})
let store = Store.create({tags: {admin: true}})
console.log(JSON.stringify(getSnapshot(store)))
applySnapshot(store, {tags: {admin: true}})
console.log(JSON.stringify(getSnapshot(store)))
applySnapshot(store, {tags: {admin: true}})
console.log(JSON.stringify(getSnapshot(store)))

Output:
{"tags":{"admin":true}}
{"tags":{}}
{"tags":{"admin":true}}
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.078% when pulling a12f0d6 on pioh:master into f773d22 on mobxjs:master.

@mattiamanzati
Copy link
Contributor

Thanks a lot for this PR!
Can you please add on the tests folder in the map.ts file a test that reproduces this bug and now passes the test? (a valid test could be the one you linked https://mattiamanzati.github.io/mobx-state-tree-playground/build/index.html#src=import%20%7B%20types%2CgetSnapshot%2C%20applySnapshot%20%7D%20from%20%22mobx-state-tree%22%0Aimport%20%7B%20inspect%20%7D%20from%20%22mobx-state-tree-playground%22%0A%0Aconst%20Store%20%3D%20types.model('Store'%2C%20%7B%0A%20%20tags%3A%20types.map(types.boolean)%2C%0A%7D)%0A%0Alet%20store%20%3D%20Store.create(%7Btags%3A%20%7Badmin%3A%20true%7D%7D)%0Ainspect(store)%0A%0AapplySnapshot(store%2C%20%7Btags%3A%20%7Badmin%3A%20true%7D%7D)%0A%0AapplySnapshot(store%2C%20%7Btags%3A%20%7Badmin%3A%20true%7D%7D)%0A )

mattiamanzati
mattiamanzati requested changes May 29, 2017
View reviewed changes
Copy link
Contributor

@mattiamanzati mattiamanzati left a comment

Choose a reason for hiding this comment

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

Please add a test for this case, so with futher changes we can be sure to not break the expected behaviour :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 93.141% when pulling 4bc510a on pioh:master into f773d22 on mobxjs:master.

@pioh
Copy link
Contributor Author

pioh commented May 29, 2017

I added one test. Thank you!

@mattiamanzati
Copy link
Contributor

Thanks for your effort!

@mattiamanzati mattiamanzati merged commit 72deec3 into mobxjs:master May 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattiamanzati mattiamanzati mattiamanzati requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pioh @coveralls @mattiamanzati