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.

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

Safe set for setDeep #471

Merged
merged 7 commits into from
May 16, 2020
Merged

Safe set for setDeep #471

merged 7 commits into from
May 16, 2020

Conversation

snewcomer
Copy link
Collaborator

close #468

This resolves issues with properties that are consumed in the template
@snewcomer snewcomer self-assigned this May 12, 2020
@basz
Copy link
Contributor

basz commented May 13, 2020

Not sure if this is the way to test this PR... Just giving some feedback...

// package.json

    "ember-bootstrap-changeset-validations": "^3.0.0",
    "ember-changeset": "git+https://github.com/poteto/ember-changeset.git#sn/safe-set-deep",
    "ember-changeset-validations": "^3.3.1",

Unfortunatly still seeing issue #468

@snewcomer snewcomer added the bug label May 15, 2020
@snewcomer
Copy link
Collaborator Author

@basz Would you mind trying again? If you don't mind inspecting the node_modules and see if these changes made it in there?

@basz
Copy link
Contributor

basz commented May 16, 2020

Hmm. Looks like the values are set correctly now... 🎉 this.changeset.changes reflect the changes made

However, for nested keys my validations don't seem to run... That could be because I'm coming from the v2 releases of

"ember-bootstrap-changeset-validations": "^2.0.0"
"ember-changeset": "^2.1.2",
"ember-changeset-validations": "^2.1.0",

anything obvious I should be aware of?

@snewcomer
Copy link
Collaborator Author

Awesome! What does it mean that nested validations don't run? If user.email are you setting email directly or setting user and expecting email validations to run?

@snewcomer snewcomer closed this May 16, 2020
@snewcomer snewcomer reopened this May 16, 2020
@snewcomer snewcomer merged commit bb21eb0 into master May 16, 2020
@snewcomer snewcomer deleted the sn/safe-set-deep branch May 16, 2020 14:22
@basz
Copy link
Contributor

basz commented May 16, 2020

I'm confused.

should the validationsMap for nested keys be like version 2.

{
  'key1': [validations],
  'key2.nested1': [validations],
}

or (nested pojo's)

{
  'key1': [validations],
  'key2': {
    'nested1': [validations],
  }
}

@snewcomer
Copy link
Collaborator Author

nested pojo's. Using the dot separated version was very limiting for cases detailed in places in #379

@basz
Copy link
Contributor

basz commented May 16, 2020

ok thanks.

We should change that on the README... Search for "specify nested keys with dot delimiters"...

@basz
Copy link
Contributor

basz commented May 16, 2020

That solved the validation when I edit a field. However, when I submit the form I do the following

this.changeset.validate().then(() => {
      alert(this.changeset.get('isInvalid'));

      console.log(this.changeset.get('errors'));
    });

Errors is then empty, alert is false, whilst it shouldn't...

@snewcomer
Copy link
Collaborator Author

@basz If there was a minimal repo of this issue, would be happy to take a look!

@basz
Copy link
Contributor

basz commented May 16, 2020

There seems to be a problem (or at least a difference between v2 and v3) with detecting the keys to be validated.

When calling this.changeset.validate() none of the validations aren run (which used to be all validtions). when I do this.changeset.validate('consumer.name.givenname') just that one and isInvalid becomes true...

@basz
Copy link
Contributor

basz commented May 16, 2020

I could do something like https://github.com/hughsk/flat on the validationMap and feed it into the validate method... But I'll wait until you confirm it isn't something validate should do by itself and is indeed a difference between v2 and v3...

@snewcomer
Copy link
Collaborator Author

@basz Interesting! We do have tests for validate/0. I'd be curious on what part of changeset.validate() isn't working.

@basz
Copy link
Contributor

basz commented May 16, 2020

Yeah, I went through the tests an found it...

It's not working on nested keys. So given;

{
  'key1': [validations],
  'key2': {
    'nested1': [validations],
  }
}

and then calling this.changeset.validate() only runs the 'key1' validations... Not sure if that is intended though... (the test '#validate/nested validates nested fields immediately' specifically calls the nested key)

@basz
Copy link
Contributor

basz commented May 16, 2020

currently working around with

validationKeys = Object.keys(flatten(dossierValidations))
      .map(key => key.slice(0, -2))
      .uniq();

this.changeset.validate(...validationKeys);

@snewcomer
Copy link
Collaborator Author

Oh interesting! Mind filing an issue? Nested keys have always been a sore spot. By moving away from dot separated (v3), we solved most of the issues. This just seems to be a missed edge by me.

@basz
Copy link
Contributor

basz commented May 16, 2020

see #478

@basz
Copy link
Contributor

basz commented May 18, 2020

Thank you @snewcomer for fixing the latest issues and taking care of this package. My upgrade path has been smooth sailing since and ember-changeset is a pleasure to work with...

@snewcomer
Copy link
Collaborator Author

Thank you for the flatten idea! Made the solution super simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with @tracked
2 participants