-
-
Notifications
You must be signed in to change notification settings - Fork 209
Add support for array of types in schema definition #78
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
Conversation
Many thanks for this PR. Can you run some benchmarks ? I like to see if there're some performance penalties between your branch and master. |
32842ed
to
e098728
Compare
Sure! I ran Before
After
Looks OK to me. If your schema doesn't contain any type arrays, the generated code is the same as before. However, the code generation itself could be a little slower. |
test/typesArray.test.js
Outdated
// console.log(e) | ||
// t.fail() | ||
// } | ||
// }) |
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.
Do you think it would be feasible to also implement this feature? If not, we should at least throw a nice error that this is not supported and test that.
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'll work on it tomorrow.
index.js
Outdated
@@ -517,6 +551,7 @@ function buildArray (schema, code, name, externalSchema, fullSchema) { | |||
condition += `Array.isArray(obj${accessor})` | |||
break | |||
default: | |||
// TODO: item.type can be an array of types. |
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.
is this todo related to the commented test?
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. At this point we need to check if item.type
is an array and then call into the switch statement from line 531 again for each element in the array. The switch statement should be factored out into a separate function.
Good work! |
e098728
to
56f2690
Compare
@mcollina I've now also added support for multiple types in tuples. |
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.
LGTM
var conditions = type.map((subType) => { | ||
return buildArrayTypeCondition(subType, accessor) | ||
}) | ||
condition = `(${conditions.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.
There is potential for an optimization here. In the current implementation the generated code checks if the item matches any of the types. The nested()
function basically performs the same check again using ajv. We could split this condition here into separate ones and use the knowledge of the item's type for the nested()
function. However, this requires some more refactoring.
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.
Feel free to experiment! We do like optimizations! :D
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'm landing this now, feel free to send a followup PR with the optimizations!
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 should be good to go if benchmarks are still ok.
Same machine and node v8.9.4 as yesterday. Looks OK.
|
Fixes #77.
This adds support for
type
to be an array of types, e.g.Support of multiple types in tuple definitions is still missing. This probably requires some refactoring of the
buildArray()
function. Nevertheless, I have prepared already a test case for tuples (currently commented out).I don't particularly like the implementation of
hasArrayOfTypes()
, so please let me know if you have any better ideas.