-
Notifications
You must be signed in to change notification settings - Fork 85
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.
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
fix for #41, reactive nested properties and dynamic properties support #54
Conversation
// console.log('BEFORE: prev[key]:', prev[key]); | ||
|
||
if (index < array.length - 1) { | ||
Vue.set(prev, key, prev[key] ? prev[key] : {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really necessary to set the value of prev[key]
to prev[key]
again? I guess this could affect performance in a negative way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vue.set is the only way to ensure deep reactivity (https://vuejs.org/v2/guide/reactivity.html).
I could save Vue.set call with a direct assignment if the object property is not responsive (check the existence of a 'ob' attribute) but I think Vue.set does that anyway.
|
||
expect(store.state.form.foo1.foo11.foo2.foo3).toBe(`fooValue`); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding an integration test, please also add / update Unit Tests to bring back coverage to 100%.
Thank you for contributing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
think I've covered all change requests |
Thanks! I'll take a closer look later this week. |
after further reflection i'dd like to cancel the pull request. |
While adding whole chunks of a chain seems like it could be excessive, adding properties at the end of the chain seems useful to me. I was thinking about pulling at least part of these changes locally into my code. Did you have reasons you thought the first use case was bad? |
@fusionbeam could you elaborate on your decision to close this pr? |
store commits via updateField now creates reactive properties across the nested chain.
also when dealing with dynamic properties (chains that are not known at compile time) this fix ensures the safety of the nested chain by creating store objects as needed.
the nested properties are made reactive by default according to vue recommendations (https://vuejs.org/v2/guide/reactivity.html#Declaring-Reactive-Properties)