added ability to have changesets extend down to nested objects #211
Conversation
|
Awesome, will take a look at this tomorrow if not later tonight. |
|
@webark Looking at this now (edit: after grabbing some food). First things first, would you mind resolving the merge conflict in package.json, and re-generating yarn.lock (with Yarn 1.3.2)? @poteto If you get the chance, please enable Circle CI builds – it doesn't seem like we can run builds on DockYard's Circle account anymore. 🙂 |
|
@nucleartide it might not be a bad idea to just pick up the "migrate to Travis" ticket. |
| let changeset = new Changeset(data); | ||
| this.set('childChangeset', changeset.get('person')); | ||
| this.render(hbs` | ||
| <h1>{{childChangeset.firstName}} {{childChangeset.lastName}}</h1> |
There was a problem hiding this comment.
Minor nitpick: please un-indent by 2 spaces here.
| let changeset = new Changeset(data); | ||
| this.set('changeset', changeset); | ||
| this.render(hbs` | ||
| <h1>{{changeset.person.firstName}} {{changeset.person.lastName}}</h1> |
There was a problem hiding this comment.
Minor nitpick: please un-indent by 2 spaces here.
| this.set('childChangeset', changeset.get('person')); | ||
| this.on('reset', () => changeset.rollback()); | ||
| this.render(hbs` | ||
| <h1>{{childChangeset.firstName}} {{childChangeset.lastName}}</h1> |
There was a problem hiding this comment.
Minor nitpick: please un-indent by 2 spaces here.
tests/unit/changeset-test.js
Outdated
| ma: { name: null } | ||
| } | ||
| }); | ||
| let dummyChangeset = new Changeset(dummyModel, dummyValidator); |
There was a problem hiding this comment.
Let's rename dummyChangeset to c for brevity. :) It'll cut down on some of the dummyChangeset noise.
| dummyChangeset.set('org.usa.ma', { name: 'Massachusetts' }); | ||
| dummyChangeset.execute(); | ||
| assert.deepEqual(get(dummyChangeset, 'change'), expectedResult, 'should have correct shape'); | ||
| assert.deepEqual(get(dummyModel, 'org'), expectedResult.org, 'should set value'); |
There was a problem hiding this comment.
These assertions seem redundant since we're already doing a .deepEqual above. Let's remove these?
There was a problem hiding this comment.
removed the tests below, and pulled out the rollback tests and have those just in their own now.
tests/unit/changeset-test.js
Outdated
| }); | ||
| let dummyChangeset = new Changeset(dummyModel, dummyValidator, dummyValidations); | ||
|
|
||
| dummyChangeset.validate('org.usa.ny').then(() => { |
There was a problem hiding this comment.
Minor nitpick: un-indent by 2 spaces please. :)
tests/unit/changeset-test.js
Outdated
| dummyChangeset.set('org.asia.sg', 'sg'); | ||
| assert.equal(dummyChangeset.get('org.asia.sg'), 'sg', 'returns newly set value'); | ||
| let childChangeset = dummyChangeset.get('org.asia'); | ||
| assert.equal(childChangeset.get('sg'), 'sg', 'child changeseet sees changed value'); |
There was a problem hiding this comment.
minor nitpick: spelling (changeseet to changeset)
| ny: null, | ||
| ma: { name: null } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Let's add a newline here just to mark different parts of the function.
tests/unit/changeset-test.js
Outdated
| let dummyChangeset = new Changeset(dummyModel, dummyValidator); | ||
| assert.equal(dummyChangeset.get('org.asia.sg'), '_initial', 'returns initial value'); | ||
| dummyChangeset.set('org.asia.sg', 'sg'); | ||
| assert.equal(dummyChangeset.get('org.asia.sg'), 'sg', 'returns newly set value'); |
| * @param {String} [scope=''] A sring that can be prepended to the key. | ||
| * @return {Array} | ||
| */ | ||
| _allKeys(object, keys = [], scope = '') { |
There was a problem hiding this comment.
Hmm, will Object.entries cause any IE issues?
There was a problem hiding this comment.
umm.. Doesn't babel still pick that up?
There was a problem hiding this comment.
It seems like Babel needs a plugin for this - the local, transpiled version still has Object.entries in the code.
|
@webark Ahh yeah, will take a look at the Travis ticket. |
webark
force-pushed
the
cleaner-branch-nested-key-support
branch
from
e0bba4e to
4cd35ea
Compare
…vered by the origianl 'deepEqual
|
k. @nucleartide I think that's all the changes. Let me know if there are any others! Glad to have movement on this! :) |
|
@nucleartide Should be active now! |
|
very nice, do you guys want to add any update to the readme/docs for this? for example do I need to create "child" changesets? will this work with validation? trying to piece together a custom form using nested pojo as model |
|
Will this be in a release soon? |
|
@thomblake Yep, will get to it sometime this week. As @erichonkanen mentioned we'll need to update docs and such. |
|
Just for everyone's info, I scheduled time on Friday to work on getting a release out. Sorry for the delay! |
|
I'd like to thank everyone involved in making this happen. Your work is much appreciated! |
|
When will this be added to the readme? |
|
Hey @nmccready, please see #256. |
Just wanted to set up a cleaner PR then #140 for expediting the adopting and reviewing of these changes.