-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add symbol() type #1562
Add symbol() type #1562
Conversation
test/types/symbol.js
Outdated
const rule = Joi.symbol().map(map); | ||
Helper.validate(rule, [ | ||
[1, true, null, symbols[0]], | ||
[symbols[0], true, null, symbols[0]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't it become symbols[2]
? Or else why accept symbols as key of a map ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah I didn't write that test as I intended to.
However, it might makes sense to work as it does, since the implementation adds all map values to the valid()
values, which is resolved before the mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I would deny symbols as keys, that doesn't make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, normally the mapping would work – it just doesn't in the case, since the symbol is also used as a value.
Anyway, I don't have a problem with removing it. We can always re-add later if it somehow makes sense.
lib/types/symbol/index.js
Outdated
|
||
describe() { | ||
|
||
const description = Any.prototype.describe.call(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That PR made me realize we could do this in ES6 now, see fd28a30.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought as well, but chose to copy the existing style.
One note: The |
Feedback has been applied. I also changed to use |
I think BigInt is an edge case here that we shouldn't necessarily care about. It's our 1st direct introduction of |
What do you think of going back to the previous stringifier ? |
There is no "hit" for browser builds since |
Which you plan to remove in 6.0 according to hapijs/hoek#268 😉 |
Wow, you're right – Hoek would drop all direct usage of util. Though probably still included as a secondary reference in other node packages. |
If I'm reading joi-browser's analysis right, it is required by hoek, isemail, and assert. Hoek and isemail can clearly remove it, and assert has a PR for it. That's a 15kB win, not much indeed, but I've been trying to see if the bundle could be optimized lately. |
We'll see about all that later... |
This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions. |
For #1420. Let me know what you think.