Fix <ReferenceInput> accepts a validate prop while it should not
#9637
Conversation
fzaninotto
force-pushed
the
referenceinput-validate
branch
from
8e77cf0
to
1723375
Compare
| @@ -84,6 +84,11 @@ export const ReferenceInput = (props: ReferenceInputProps) => { | |||
| filter, | |||
| }); | |||
|
|
|||
| if (props.validate) { | |||
There was a problem hiding this comment.
We could also modify the ReferenceInputProps type:
extends Omit<InputProps, 'validate'>
There was a problem hiding this comment.
I don't think that would be a better DX: Developers would see a failed compilation because of validate, without knowing what to do instead.
There was a problem hiding this comment.
I would also suggest to remove the property from the type.
We just stumbled into a bug due to this change and it was not detected by our CI. We do not have tests of the edit form and only learnt about this through a bug report, because the types were still valid. However, when executing the code it is now throwing an error.
I think a type adjustment would be best to make this change visible, as it will break the application in any case:
There was a problem hiding this comment.
If we change the type it will be a BC, right?
This two changes break something, so IMO we should update the types in V5 and revert this PR to put it in V5 too.
There was a problem hiding this comment.
The bug in your code wasn't caused by this change, it was revealed by this change.
As explained, a type adjustments isn't a better DX IMO. Unless we add a "never" type and explain it with JSDoc?
There was a problem hiding this comment.
Well, yes and no. You could say there was a bug, but defining the validation prop on the <ReferenceInput> did not have any effect and it was also defined on the <AutocompleteInput>. The validation was working and one validation prop was unnecessary, but that's not really a bug, is it? But now the application/page is crashing, which imho is more severe and I would call this a bug that was introduced by this change.
There was a problem hiding this comment.
You're right. We shouldn't throw an error in production-only in development. I'll open a new PR to fix it. We won't introduce a type check until v5 though, as it is a breaking change.
## Problem Since #9637, using `<ReferenceInput validate>` throws an error at runtime. ## Solution Catch the problem at build time for TS users.
## Problem Since #9637, using `<ReferenceInput validate>` fails at runtime. this is unexpected ## Solution Fail at build time instead. This may break existing apps, so I'm pulling against `next`.
## Problem Since #9637, users see an error in production while the app isn't broken ## Solution Only throw in development.
Closes #9568