-
-
Notifications
You must be signed in to change notification settings - Fork 208
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
Adding support for TypedArrays #640
base: master
Are you sure you want to change the base?
Conversation
index.js
Outdated
@@ -586,7 +590,7 @@ function buildArray (context, location) { | |||
` | |||
|
|||
functionCode += ` | |||
if (!Array.isArray(obj)) { | |||
if (!Array.isArray(obj) && !(obj != null && (${supportedTypedArrays.map(type => ' obj.constructor.name === \'' + type + '\' ').join('||')}) )) { |
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.
ugh. It would be better to put the array in a "global" scope and make an indexOf by the obj.constructor.name than this imho.
I think this will slow down array significantly.
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.
I am not familiar with perf implications here.
Would Array.includes also be worth testing?
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.
This would not work in places where we use Ajv. To test it put an array under anyOf, oneOf or if.
PR:
MASTER:
|
It seems like there is no way to solve this? |
I have added support for
Uint8Array
with a test showing that it works as expected. No additional types have been created, this works by simply treating aUint8Array
like a normalarray
. Afaik, you cannot create a nested array this way so the changes are quite limited.Support for more types of
TypedArray
is easy to add but as I worked on this issue further I realised that there may be wider implications for this:TypedArray
should be supported?Related to - #626 - I haven't verified the performance difference either, I am assuming this can only come from removing type checks?
Checklist
npm run test
andnpm run benchmark
and the Code of conduct