Skip to content

Conversation

@ispyinternet
Copy link

I've added tests and updated the readme if you are happy to include this feature.

@coveralls
Copy link

coveralls commented Feb 5, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 5f2f7a7 on ispyinternet:master into f83d71b on icebob:master.

@icebob icebob self-requested a review February 5, 2019 19:25
Copy link
Owner

@icebob icebob left a comment

Choose a reason for hiding this comment

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

Thank @ispyinternet, great PR, but this solution causes performance degradation. I think there is no reason to manage custom messages in validation time because it can be managed in compilation time and performance won't be affected. What do you think? Could you modify the PR?

@ispyinternet
Copy link
Author

@icebob I've tried to adapt. Please review.

lib/validator.js Outdated
messages[schemaType.type] = this.messages[schemaType.type];

Object.keys(schemaType).forEach(rule => {
if (rule == "type" || rule == "items" || rule == "props" || rule == "messages") return;
Copy link
Owner

Choose a reason for hiding this comment

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

Could you explain what is this code?

Copy link
Author

@ispyinternet ispyinternet Feb 6, 2019

Choose a reason for hiding this comment

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

In order to remove check from run time we have to prepare all possible message types at compile time. These include:

‘required’
‘string’ | ‘number’ | ‘array’ etc (i.e type)

Then we analyse the additional rules to see what other messages need to be added. When analysing the rule object, obviously we ignore the other properties that we know are not additional rules, i.e. 'type', 'items' for arrays, 'props' for objects and our new 'messages' property.

Copy link
Author

Choose a reason for hiding this comment

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

I've just changed the if statement to use key in obj style rather than truthy test

@icebob
Copy link
Owner

icebob commented Feb 6, 2019

Good job, the performance has been improved! On my PC 5.3M ops/sec. The master branch is 5.2M 👍

@icebob
Copy link
Owner

icebob commented Feb 6, 2019

please don't force push commit because i'm trying to linting the codes.

@ispyinternet
Copy link
Author

sorry, I wont anymore

Copy link
Owner

@icebob icebob left a comment

Choose a reason for hiding this comment

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

Great! So is it done? Can I merge?

@ispyinternet
Copy link
Author

All tests are passing, not sure if you want to review performance or do any further sanity checks. Some unexpected changes to review.

@icebob
Copy link
Owner

icebob commented Feb 7, 2019

The compile is very bad. It comes with the latest commit. I think due to Object.keys because it is a very expensive function.
This PR:
√ compile & validate 28,840 rps
√ validate with pre-compiled schema 5,359,590 rps
√ validate with wrong obj 668,274 rps

Master:
√ compile & validate 130,815 rps
√ validate with pre-compiled schema 5,123,820 rps
√ validate with wrong obj 657,710 rps

@ispyinternet
Copy link
Author

Also for every rule we are now iterating over all the default messages. Previously only iterates if user supplied. Perhaps an overwriting merge of the two objects. I’ll make the update. Is there a command to run so I can get the metrics to report back?

@icebob
Copy link
Owner

icebob commented Feb 7, 2019

I've added a condition, so only handle custom messages if there is messages property in schema.

@icebob
Copy link
Owner

icebob commented Feb 7, 2019

Now it's much better.
It doesn't affect the performance without messages, and with it now is ~70k

√ compile & validate 119,438 rps
√ compile & validate with custom messages 69,858 rps
√ validate with pre-compiled schema 5,220,102 rps
√ validate with wrong obj 660,502 rps

By the way, you can check perf with npm run bench

@ispyinternet
Copy link
Author

Nice! Why did you remove Object.keys(this.messages) from constructor? Now it is called everytime a message property is encountered - negligible admittedly!?

Also did you see the slight changes to lib/rules/string.js and lib/rules/email.js?

@icebob
Copy link
Owner

icebob commented Feb 7, 2019

Because there is not relevant difference. With Object.keys ~68k, with static array ~70k. But I'm checking again.

In string & email files I made some linting in regexp.

@icebob
Copy link
Owner

icebob commented Feb 7, 2019

@ispyinternet I put back the key array. It gives +10%. So now I think it's ready & performant. Could you add this new feature to the README too with examples?

@ispyinternet
Copy link
Author

I already updated the README its the last section

@icebob
Copy link
Owner

icebob commented Feb 7, 2019

Really, sorry :)

@icebob icebob merged commit 3f41557 into icebob:master Feb 7, 2019
@icebob
Copy link
Owner

icebob commented Feb 7, 2019

Thank you for your work.

@ispyinternet
Copy link
Author

Thanks for your help too!

@ispyinternet
Copy link
Author

you can close #28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants