-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
Improve performance for const schemas #511
Conversation
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
index.js
Outdated
json += JSON.stringify(${input}) | ||
` | ||
} else if ('const' in schema) { | ||
code += ` | ||
} else if ('const' in schema) { |
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 removed.
index.js
Outdated
const defaultValue = schema.properties[key].default | ||
let defaultValue = schema.properties[key].default | ||
if (isRequired) { | ||
defaultValue = defaultValue !== undefined ? defaultValue : schema.properties[key].const |
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.
const shouldn't work as a default value. If you define a schema constant property, it must const value, whatever value the user passes.
- Removed switchTypeSchema - Use of variable isRequired where defined - Rebase to PR #510 - Remove use of default value for const
index.js
Outdated
` | ||
} else if (required.includes(key)) { | ||
} else if (isRequired && isConst) { |
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 work in a different way.
- You should handle value serialization only in one place otherwise you will have code duplication and can make an error. In your case, you miss the null check here. So please handle all schema serialization logic inside buildValue function.
- We don't need to do this check for const
Line 365 in 844eba5
if (obj[${sanitized}] !== undefined) {
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.
Null checks you mean this?
const schema = {
"type": "object",
"properties": {
"foo": {
"const": "bar"
}
}
}
stringify({ "foo": "bar" }) // { "foo": "bar" }
stringify({ "foo": "baz" }) // { "foo": "bar" }
stringify({ "foo": 1 }) // { "foo": "bar" }
stringify({}) // { }
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.
To answer 2.
If I don't have that checks how can I handle this?
const schema = {
"type": "object",
"properties": {
"foo": {
"const": "bar"
}
}
}
stringify({}) // { }
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 think that the const value shouldn't work as a default value. It shouldn't have any other additional behavior.
const schema = {
"type": "object",
"properties": {
"foo": {
"const": "bar"
}
},
"required": ['foo']
}
stringify({}) // throw new Error('foo is required')
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 OK with that, but I remember that someone agrees on this comment.
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, you are right, it should. From my point of view. But let's wait for other opinions.
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.
If you are waiting for me, I'm leaving this to you.
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 would like to hear @climba03003 opinion as it was his suggestion #505 (comment).
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.
When the schema said the property is required, and missing one from the input.
It just convenient to always put the value there since it is required
.
Just like why we always put the const
value regardless the input.
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 do not block if it turns to throw.
It consistences with the current required
handling for other properties.
I read the code. Now it is like 80% reformatting. 19% tests and 1% change. And somehow i dont see any code which is responsible for comparing const with the actual value. I guess ajv does the validation? I cant follow, lol. How i read it, it is always serializing const no matter what the actual value is. I am not that into the codebase so i guess i am out of here. |
@Uzlopak Yes my bad, I don't know why I have the format changes since I don't even have the formats on save options. |
@Uzlopak yes you are right. The main idea of #505 (review), #505 (comment) was to ignore the actual value and always insert the const value. I agree with this point of view because fjs is not a validator and if we can do something without validation we don't have to do validation. If you don't agree please share your thoughts. |
@DanieleFedeli Null check. const schema = {
"type": "object",
"properties": {
"foo": {
"nullable": true
"const": "bar"
}
}
}
stringify({foo:null}) // {foo:null} const schema = {
"type": "object",
"properties": {
"foo": {
"type": [...whatever, 'null']
"const": "bar"
}
}
}
stringify({foo:null}) // {foo:null} |
@DanieleFedeli, Can you revert the formatting changes? It would be easier to review for other people. |
I can try |
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
@climba03003 are you ok for this to land? |
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.
LGTM
Checklist
npm run test
andnpm run benchmark
and the Code of conduct
As per issue #502, when using a const schema, the performance degrades. This is the continuation of #505.
Benchmark
Pre PR
![Screenshot 2022-08-16 alle 10 50 37](https://user-images.githubusercontent.com/37077048/184842079-5b642410-7897-45e5-936f-e374b75abff6.png)
Post PR
![Screenshot 2022-08-25 alle 18 46 08](https://user-images.githubusercontent.com/37077048/186725944-119975ce-b2c4-4790-a193-b75f35efd821.png)
This PR supports nullable values:
Or it coerces the value to the const property if it is required