WIP: Support nested keys

[poteto]

No description provided.

[dpigera]

@poteto Do you have an ETA on when this will get merged ?

[poteto]

Sorry for the delay! Will get this over the line shortly.

[majnikov]

Hi @poteto , what about this PR?

[JCBarry]

Hi there! Just ran into needing this as well. Is there a relevant workaround available?

[jbailey4]

@poteto Ran into this use case today, any idea when this could be merged? Glad to help with any work that may be needed.

[poteto]

Sorry, work keeps getting in the way :( If someone would like to help me get this over the line I would be very grateful! This is how I'm thinking this feature will be implemented:

1. Check if a key being set is nested
2. Check if the change object has the object(s) required to set that value
   • true: use makeNestedObject to create the object(s)
   • false: no-op
3. Set the value as normal
4. Tests

[jbailey4]

@poteto I'll take a look at this tonight and see where I get. The work should be based on what you already have here in the feature/support-nested-keys branch, correct?

[poteto]

Yup!

[jbailey4]

@poteto

Update:

It seems changeset relies on the setUnknownProperty to kick off the validation and then ultimately apply the change/error, but this isn't happening when attempting to set a nested prop. Instead, Ember is calling unknownProperty with the nested object name (first item in the path), which then calls _valueFor. This returns the underlying nested object, which Ember then basically continues on setting the intended value, without changeset ever knowing.

As a baseline first test I have the following:

set(dummyModel, 'contact', { phoneNumber: 1112223333 });

let expectedResult = [ { key: 'contact', value: { phoneNumber: 9998887777 } } ];
let dummyChangeset = new Changeset(dummyModel);

dummyChangeset.set('contact.phoneNumber', 9998887777);

assert.deepEqual(get(dummyChangeset, 'changes'), expectedResult, 'precondition'));

[poteto]

I had another go at it with some inspiration from @jgwhite.

However there are still some weird issues I'm trying to figure out

• Validate nested key
• Be able to set more than 1 root nested key
• Finish documentation

If anyone has any clues, please let me know...

[willrax]

hey @poteto, I'm trying to work on the points you wrote above. We would love to have nested validations in our application for a few "interesting" forms.

Are you committed to having this structure for the validations?

{
 name: validateWhatever(),
 "address.street_name": validateWhatever()
}

It seems at the moment you can't access keys that contain dots within ember objects, so accessing the validator function is tricky.

I'm @willrax on Slack if you want to discuss in more detail, but would you consider:

{
  name: validateWhatever(),
  address: {
    street_name: validateWhatever()
  }
}

as an alternative?

[poteto]

I think the string option is the general practice in other ember validation libraries. I'm unsure what kind of extra complexity the object form would introduce, although in theory I think it should be the same code to retrieve the function via Ember.get

[willrax]

Ah right, If other libraries do it, it's probably best to stick to the convention then. I'll attempt to get the validations lookups (with the string keys) and that debugger in the tests at least hitting today.

[willrax]

Would you mind if we rebased (or merged) master in to this branch?

[willrax]

Had a quick look at some other libraries and it looks like internally they expand the string keys in to an object:

https://github.com/offirgolan/ember-cp-validations/blob/f0f8a92fdc2d09721b7acc4afd61fdbfae33e030/addon/validations/factory.js#L192

This gives people the string keys but then allows you to access the keys through the get set etc.

This seemed to not matter in the end. Doing validationsMap["org.ny.ny"] works fine.

[poteto]

@willrax I don't mind! If you'd like, please open a PR merging into this branch feature/support-nested-keys

Also, huge thanks for stepping up to take this over the line!

[minhnq013]

Hello, Just wondering if there is any news regarding this.

I am currently encountering this issue and there are workaround but it would be awesome if we can do nested property validation.

[willrax]

hey @minhnq013 Yep, still working on this 👍

[klclee]

HI @willrax @poteto looks like this is really close to done? Anything I can do to move this along? Could really do with it then having individual changeset for each nested model 👍

[RobbieTheWagner]

Would also love to get this merged. I've got arrays in my changeset that don't rollback, which is annoying. Let me know if there is any way I can help!

[webark]

@nrcjbarry does my fork in #194 help? If not, what's the data structure?

[RobbieTheWagner]

Bump, need to get the amazing work by @webark reviewed and merged! Would be great to add him as a collaborator, and I am happy to review and help out any way I can!

[webark]

Still getting that

Cannot find module 'is-absolute'
Error: Cannot find module 'is-absolute'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/home/ubuntu/ember-changeset/node_modules/npm/node_modules/which/which.js:12:18)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)

error every other circle ci test. I'm unsure what in circle ci is causing this error.

[steverhoades]

Curious if this feature has made any additional headway and if not what are the remaining items to be completed? Perhaps I can assist as I'd really like to see this introduced into Changeset.

[webark]

@steverhoades as far as I'm concerned, everything is working as expected. We've gone back and forth on some extra tests, and all of those have been added and resolved. I am unsure as to why the circle ci build errors are occurring, but I didn't run into any errors locally, or with Travis. THere are some merge conflicts, but those are easier for a maintainer to resolve due to the back and forth with doing PR's on to side branches, but if that is a blocker I am more the willing to issue a pr against the side branch to resolve those issues.

[RobbieTheWagner]

@webark I vote we just remove circle and rely on travis, since that is the supported CI for Ember addons out of the box anyway.

[steverhoades]

@webark @rwwagner90 I'm getting the following error when attempting to use this branch:

Error: Property set failed: object in path "mykeyname" could not be found or was destroyed.
    at new EmberError (error.js:38)
    at setPath (ember-metal.js:2764)
    at set (ember-metal.js:2692)
    at Class.addError (index.js:341)
    at Class._setProperty (index.js:538)
    at Class._validateAndSet (index.js:467)
    at index.js:292
    at Array.map (<anonymous>)
    at Class.validate (index.js:291)
    at Class.submit (component.js:58)

I am running Ember 2.15 as well as ember-changset-validations v1.2.8. I've also noticed that this branch is quite a bit behind master - does anyone have a rebased version of this branch somewhere? My first attempt at rebasing didn't result in working code.

[steverhoades]

@webark @rwwagner90 Please disregard my last message. I apologize, when i updated ember-changeset-validations it removed my link to ember-changeset. I do not see this error with this branch.

[steverhoades]

@webark @rwwagner90 @poteto I have verified that this works beautifully in our app. Would really like to see this released - how can I help?

[poteto]

I would like to remove CircleCI as well, but in a separate PR. Does anyone have the bandwidth to help with rebasing this and resolving merge conflicts?

[steverhoades]

@poteto I'm attempting this now. If I can get something working i'll submit a PR.

[steverhoades]

@poteto @webark @rwwagner90 I've pushed a rebased version of this branch, tests are passing but I had to force update my branch and it will not cleanly integrate into this branch. I'm gonna give it another shot to see if I can get a cleaner result.

See steverhoades@44aefc0

[steverhoades]

@poteto @webark @rwwagner90 Ok, looks like this attempt is much much cleaner on the rebase side of things.. will provide PR.

steverhoades@d13917d

[nucleartide]

Hey folks, since this is the issue/PR with the most activity, I'll try to find some time to review this and get it merged. As a newly appointed maintainer, I'd also like to start doing monthly releases in order to close out all the issues/PRs.

Hope that's okay with everyone!

[RobbieTheWagner]

@nucleartide sounds great! Glad to have you on board 😄

[nucleartide]

So I notice that this PR has some changes that aren't necessarily related to nested keys support. I'd like to break this PR into smaller ones, so the actual change is easier to review. 🙂

It seems like yarn test is also failing on master – will take a look at that first.

[webark]

@nucleartide I was having issues with circle CI tests not passing, so I tried updating yarn and ember/ember-cli to see if that would resolve the issue, which it didn't. Feel free to pull out all the non-code changes, and put that into a separate PR.

[webark]

some of the other stuff is coming through due to a poor merge confict.

[webark]

@nucleartide I just created #211

#211 is what this PR should be if it was able to be done in a cleaner way. (doing PR's against long-lived branches that are not up to date with master.. has complexities)

[poteto]

Closed by #211