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.
Why wouldn't it become symbols[2] ? Or else why accept symbols as key of a map ?
There was a problem hiding this comment.
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.
Then I would deny symbols as keys, that doesn't make sense to me.
There was a problem hiding this comment.
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.
That PR made me realize we could do this in ES6 now, see fd28a30.
There was a problem hiding this comment.
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.