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.

Sign up for GitHub

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

Validate changeset getter #54

Merged
merged 3 commits into from
May 26, 2020
Merged

Validate changeset getter #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d12fc5a
Add failing test for validating changeset getter
andreyfel May 25, 2020
30c0b2a
Try to get a property from the changeset
andreyfel May 25, 2020
18cba99
Don't take cached value in _valueFor
andreyfel May 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,12 +791,11 @@ export class BufferedChangeset implements IChangeset {
*/
_valueFor(key: string): any {
let changes: Changes = this[CHANGES];
let errors: Errors<any> = this[ERRORS];
let content: Content = this[CONTENT];

if (errors.hasOwnProperty(key)) {
let e: Err = errors[key];
return e.value;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts on removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put it in a separate commit with my thoughts.
Basically I think that this "caching" is redundant here and can be error-prone.
I initially put the logic of taking the property after that section and it didn't work because the cache wasn't invalidated when related fields were changed. So, I've put it before that block and then removed this if section in a separate to get your feedback about that.

If you think it is useful, I can just drop the last commit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! Looks like we don't have a test for this case. If we try and validate a key, is it possible the validation results only lives in ERRORS? Perhaps we can add a test case to check this assumption?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code I removed just accesses the value, contained in ERRORS object, not the validation results. The value can be always accessed from the changes or the underlying object. Therefore I consider this thing as a performance optimization, not the necessary functionality.

Maybe I'm missing something?

In any case, this change is not really tied to the original issue I'm trying to address. I can extract it to a separate PR or just drop it.

What do you think about the change in 30c0b2a?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate PR may be good unless we can confirm with a test. The changes you have for the specific issue seem 💯

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value can be always accessed from the changes or the underlying object

That is - a test confirming that adding an error will result in the right validations. (perhaps we already have one)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean addError API, there is a bunch of tests tagged with #addError.

// Derived property, i.e. property defined in the changeset itself
if (Object.prototype.hasOwnProperty.apply(this, [key])) {
return this[key];
}

// 'person'
Expand Down
54 changes: 54 additions & 0 deletions test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1942,6 +1942,60 @@ describe('Unit | Utility | changeset', () => {
]);
});

it('#validate changeset getter', async () => {
class MyModel {
isOptionOne = false;
isOptionTwo = false;
isOptionThree = true;
}

const Validations = {
isOptionSelected: (newValue: boolean) => {
return newValue === true ? true : 'No options selected';
}
};

function myValidator({
key,
newValue,
oldValue,
changes,
content
}: {
key: string;
newValue: unknown;
oldValue: unknown;
changes: any;
content: any;
}) {
let validatorFn = get(Validations, key);

if (typeof validatorFn === 'function') {
return validatorFn(newValue, oldValue, changes, content);
}
}

const myObject = new MyModel();
const myChangeset = Changeset(myObject, myValidator, Validations);

Object.defineProperty(myChangeset, 'isOptionSelected', {
get() {
return this.get('isOptionOne') || this.get('isOptionTwo') || this.get('isOptionThree');
}
});

await myChangeset.validate();
expect(myChangeset.isInvalid).toEqual(false);

myChangeset.set('isOptionThree', false);
await myChangeset.validate();
expect(myChangeset.isInvalid).toEqual(true);

myChangeset.set('isOptionTwo', true);
await myChangeset.validate();
expect(myChangeset.isInvalid).toEqual(false);
});

it('#isInvalid does not trigger validations without validate keys', async () => {
const model = { name: 'o' };
const dummyChangeset = Changeset(model, lookupValidator(dummyValidations));
Expand Down