-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix: fixes #3692 - simple discriminator now bypasses isValid() calls in getMatchingOption and getClosestMatchingOption for improved performance #3845
Conversation
…() calls in getMatchingOption and getClosestMatchingOption.ts for improved performance
Compares the value of `discriminatorField` within `formData` against the value of `discriminatorField` within schema for each `option`. Returns index of first `option` whose discriminator matches formData. Returns `undefined` if there is no match. | ||
|
||
This function does not work with discriminators of `"type": "object"` and `"type": "array"` |
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 quite struggled trying to make this description as understandable and straght-forwards as possible.
test('const discriminator', () => { | ||
expect( | ||
getOptionMatchingSimpleDiscriminator( | ||
{ foo: 'foo' }, | ||
[{}, { type: 'object', properties: { foo: { const: 'foo' } } }], | ||
'foo' | ||
) | ||
).toEqual(1); | ||
}); | ||
|
||
test('enum discriminator', () => { | ||
expect( | ||
getOptionMatchingSimpleDiscriminator( | ||
{ foo: 'foo' }, | ||
[{}, { type: 'object', properties: { foo: { enum: ['bar', 'foo'] } } }], | ||
'foo' | ||
) | ||
).toEqual(1); | ||
}); | ||
}); |
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.
Perhaps there are some other relevant cases that I'm missing
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.
You just need to figure out the test coverage
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.
Done
import { PROPERTIES_KEY } from './constants'; | ||
import { RJSFSchema, StrictRJSFSchema } from './types'; | ||
|
||
export default function getOptionMatchingSimpleDiscriminator<T = any, S extends StrictRJSFSchema = RJSFSchema>( |
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.
Please add the same documentation you put into the docs above here.
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.
done
@michal-kurz It looks like your changes causes line |
Reasons for making this change
fixes #3692
Checklist
npm run test:update
to update snapshots, if needed.