Remove joi to shrink module size #348
Conversation
There was a problem hiding this comment.
Awesome work!! 👏 lot of people will like this 🎉
Some feedback:
a) Since the validation code is in our plate now, can you add tests to it?
b) We'll probably upgrade major version with this, because we are modifying joi ValidationError by Error (and messages even if no one should depend on them), it'd be nice to have proper error messages so we don't need to improve the impl later. I was thinking, maybe each schema item could have the validation function + validation error message, ie: iat: { validate: isNumber, errorMessage: '"iat" must be a number'}, that way you don't need to do smarter stuff but messages are meaningful.
About Expected object error, you could have a main object schemas: { payload, signOptions } and use the name of it when you call validate ie: validate('payload', ...) so you can raise a better message. These are just ideas to keep it simple but nicer for consumer, be free to implement it in different way.
Also, good idea adding the cost of modules, that could be part of a different PR, but it is fine for me to have it together.
sign.js
Outdated
|
Changes sound good, as soon as I get a chance I'll update the PR. |
test/schema.tests.js
Outdated
There was a problem hiding this comment.
Wasn't sure how to get these tests to pass, I assume I'm missing some required configuration for the ES* algorithms.
There was a problem hiding this comment.
You need to use different certificates for RS and ES. Probably it's that.
Take a look to: https://github.com/auth0/node-jsonwebtoken/blob/master/test/jwt.asymmetric_signing.tests.js#L13
|
So it took a little longer than I expect to get back around to doing this but I think I've made all of the suggested changes. |
test/schema.tests.js
Outdated
There was a problem hiding this comment.
You need to use different certificates for RS and ES. Probably it's that.
Take a look to: https://github.com/auth0/node-jsonwebtoken/blob/master/test/jwt.asymmetric_signing.tests.js#L13
test/iat.tests.js
Outdated
There was a problem hiding this comment.
Why do you remove it? is it because with this PR it is tested in the schema test? (if so, I'm ok with that)
There was a problem hiding this comment.
Yep, that's correct.
Results before any changes ┌─────────────┬────────────┬───────┐ │ name │ children │ size │ ├─────────────┼────────────┼───────┤ │ joi │ 4 │ 3.12M │ <--!!! ├─────────────┼────────────┼───────┤ │ jws │ 5 │ 0.18M │ ├─────────────┼────────────┼───────┤ │ lodash.once │ 0 │ 0.01M │ ├─────────────┼────────────┼───────┤ │ ms │ 0 │ 0.01M │ ├─────────────┼────────────┼───────┤ │ xtend │ 0 │ 0.00M │ ├─────────────┼────────────┼───────┤ │ 5 modules │ 9 children │ 3.32M │ └─────────────┴────────────┴───────┘
Dramatically reduces the module size without breaking ES5 compatability - ┌──────────────────────┬────────────┬───────┐ │ name │ children │ size │ ├──────────────────────┼────────────┼───────┤ │ jws │ 5 │ 0.18M │ ├──────────────────────┼────────────┼───────┤ │ lodash.includes │ 0 │ 0.02M │ ├──────────────────────┼────────────┼───────┤ │ lodash.once │ 0 │ 0.01M │ ├──────────────────────┼────────────┼───────┤ │ lodash.isinteger │ 0 │ 0.01M │ ├──────────────────────┼────────────┼───────┤ │ ms │ 0 │ 0.01M │ ├──────────────────────┼────────────┼───────┤ │ lodash.isplainobject │ 0 │ 0.01M │ ├──────────────────────┼────────────┼───────┤ │ xtend │ 0 │ 0.00M │ ├──────────────────────┼────────────┼───────┤ │ lodash.isstring │ 0 │ 0.00M │ ├──────────────────────┼────────────┼───────┤ │ lodash.isboolean │ 0 │ 0.00M │ ├──────────────────────┼────────────┼───────┤ │ lodash.isnumber │ 0 │ 0.00M │ ├──────────────────────┼────────────┼───────┤ │ lodash.isarray │ 0 │ 0.00M │ ├──────────────────────┼────────────┼───────┤ │ 11 modules │ 5 children │ 0.25M │ └──────────────────────┴────────────┴───────┘
|
Thanks for the pointer on ECDSA signing, I've updated the tests to use the other key where appropriate. |
ziluvatar
left a comment
There was a problem hiding this comment.
Awesome, we have our gold PR for the major update. I'll try to find time this week or next week to merge all wanted PRs for next major, v8 without JOI is coming... 👏
As noted in #343 & #292 the specific version of
joiis bloating the package size. However upgrading to the latest version would break ES5 compatibility.This PR shrinks the module size from 3.12M to 0.25M by replacing joi with bespoke implementation using lodash helpers.
N.B. To keep the implementation as simple as possible, these changes mean that the validation error messages are marginally less helpful e.g. -
"expiresIn" must be a numberversus"expiresIn" is not the correct typeThis should also solve #334.