Avoid unnecessary inference of stack for label #213
Conversation
- For `ow.isValid`, it's acutually not necessary to inference the stack for the label because the message won't actually be used. - For the check of elements, e.g. `ow.object.valuesOfType`, `ow.array.ofType`, `ow.map.keysOfType`, `ow.map.valuesOfType`, `ow.set.ofType`, the inference of the label can also be avoided. - The original reason why I want to avoid the inference of label is not because of performance. It was because `Error.prototype.stack` is not in the standard. And it isn't available in [Goja](https://github.com/dop251/goja). When `ow` tries to use `callsites` to inference the stack, it will cause an error `TypeError: Cannot read property 'slice' of undefined or null` because of [this line](https://github.com/sindresorhus/callsites/blob/6dc8cb32082c54c65c0e38672e00162c9362be22/index.js#L6). For most cases, users of `ow` can explicitly specify the label to avoid the inference such as `ow(value, 'value', ow.string)`. However, the cases mentioned above is out of the control of the users. - After the change in this PR, the error message is changed for some cases. I'm not sure if it is acceptable. If anyone has a better idea for this problem, feel free to commit to my branch or close my PR and create your own. :)
test/map.ts
Outdated
| @@ -178,11 +178,11 @@ test('map.valuesOfType', t => { | |||
|
|
|||
| t.throws(() => { | |||
| ow(new Map([['unicorn', '🦄']]), ow.map.valuesOfType(ow.number)); | |||
| }, '(Map) Expected argument to be of type `number` but received type `string`'); | |||
| }, '(Map) Expected `item` to be of type `number` but received type `string`'); | |||
There was a problem hiding this comment.
This is incorrect since we use backticks to indicate variable names or types.
It should ideally be:
| }, '(Map) Expected `item` to be of type `number` but received type `string`'); | |
| }, '(Map) Expected item 0 to be of type `number` but received type `string`'); |
There was a problem hiding this comment.
Agree. I'll take some time to see how to achieve that.
|
I rolled back some changes and focus on |
source/utils/of-type-with-name.ts
Outdated
|
|
||
| @hidden | ||
| @param source Source collection to test. | ||
| @param name The name to call the collection of values, such as "values", "keys". |
There was a problem hiding this comment.
I don't think this needs to be a separate assertion. You can just add an optional parameter to of-type.
There was a problem hiding this comment.
I updated the PR.
After we migrate all usages of ofType to specify the optional name parameter, we can change that to be required.
There was a problem hiding this comment.
After we migrate all usages of ofType to specify the optional name parameter, we can change that to be required.
Can you add a code comment about that? So we don't forget.
There was a problem hiding this comment.
Sure, updated the comment.
ow.isValid, it's acutually not necessary to inference the stack for the label because the message won't actually be used.ow.object.valuesOfType,ow.array.ofType,ow.map.keysOfType,ow.map.valuesOfType,ow.set.ofType, the inference of the label can also be avoided.Error.prototype.stackis not in the standard. And it isn't available in Goja. Whenowtries to usecallsitesto inference the stack, it will cause an errorTypeError: Cannot read property 'slice' of undefined or nullbecause of this line. For most cases, users ofowcan explicitly specify the label to avoid the inference such asow(value, 'value', ow.string). However, the cases mentioned above is out of the control of the users.