Added option to allow merging invalid changesets #74
Conversation
|
I was actually thinking merge would merge changesets regardless of validity by default. I think it was too heavy handed in the beginning - the user should be able to decide since the changeset can only be executed if it is valid. What do you think? |
|
Yeah, totally, I'm happy with that. PR updating... |
|
Hmm so some thoughts on merging logic: Let's assume we have the following (taken from readme): let changesetA = new Changeset(user, validatorFn);
let changesetB = new Changeset(user, validatorFn);
changesetA.set('firstName', 'Jim');
changesetA.get('errors') // [{key: "firstName", value: "Jim", validations: ["Firstname cannot be Jim"]}]
changesetB.set('firstName', 'Jimmy');
changesetB.set('lastName', 'Fallon');
let changesetC = changesetA.merge(changesetB);Now if we merge
Thoughts? |
|
Essentially what I'm getting at is, after merging you shouldn't end up with changes on |
|
Yup agreed, I think it should work the same way |
| let errorsB = get(changeset, ERRORS); | ||
| let mergedErrors = pureAssign(errorsA, errorsB); | ||
| newChangeset[ERRORS] = mergedErrors; | ||
| newChangeset.notifyPropertyChange(ERRORS); |
There was a problem hiding this comment.
Sidenote: use this._notifyVirtualProperties();
|
I think that makes sense, I'll reject keys from errors on A which are present changes of B. That actually would make logical sense in my form because each section is merged from top to bottom. |
|
@poteto I think this solves it ^^ |
|
|
||
| ```js | ||
| let changesetA = new Changeset(user, validatorFn); | ||
| let changesetB = new Changeset(user, validatorFn); | ||
| changesetA.set('firstName', 'Jim'); | ||
| changesetA.get('errors') // [{key: 'firstName', value: 'Jim', validations: ['Firstname cannot be Jim, sorry Jim']}] |
There was a problem hiding this comment.
Nitpick: prefer having spaces within {}, e.g. [{ key: ... }]
There was a problem hiding this comment.
I'm not sure this addition helps with understanding how merge works, feels like it could be confusing
There was a problem hiding this comment.
Cleaned that up
|
Ready to roll |
| changesetB.set('lastName', 'Fallon'); | ||
| let changesetC = changesetA.merge(changesetB); | ||
| changesetC.execute(); | ||
| user.get('firstName'); // "Jimmy" | ||
| user.get('lastName'); // "Fallon" | ||
| ``` | ||
|
|
||
| When merging, the changeset being merged in takes precedence. For example: if `B` contains errors, the errors from `B` will be merged, and the invalid attributes on `B` will not be merged from `A` even if they're valid on `A`. Likewise, if `A` contains errors on attributes which are valid on `B`, the value from `B` will be used and the errors for those keys will not be merged from `A`. This is to prevent the new changeset from containing `changes` and `errors` on the same keys, which is a guarantee of a normal changeset. |
There was a problem hiding this comment.
This is still confusing, in fact I think it would be better to just remove all the changes to the readme and only keep the change on L383. I feel like this is just implementation detail that the end user doesn't need to know about.
|
Sorry about the delay, was out the whole day! Just a minor update to the readme and we're good to go |
|
@poteto |
?
|
Yes, thanks for this! |
From discussion in #66