-
-
Notifications
You must be signed in to change notification settings - Fork 617
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(matchedData): add generic types for return #1246
feat(matchedData): add generic types for return #1246
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.
Looking good.
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.
thanks, this is an excellent enhancement 馃槂
@@ -49,8 +49,7 @@ export function matchedData( | |||
.filter(validityFilter) | |||
.map(field => field.instance) | |||
.filter(locationFilter) | |||
.reduce((state, instance) => _.set(state, instance.path, instance.value), {}) | |||
.valueOf(); |
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.
Had to make a fix here; missed how _(...).reduce()
automatically unwraps the result value, meaning that this valueOf()
was incorrect here.
Description
As brought up in #1041, it's currently hard to type the result of
parsedData
. It comes with all fields set asany
unless we do a type assertion via theas
keyword.However, this syntax overrides all type-checking. To mitigate that, this PR adds an optional generic type to the
parsedData
function signature, so it becomes:Allowing users to keep using
matchedData
with the same signature as before:but to provide a type signature of the expected return (if it's been validated) if they'd like:
There are no breaking changes, since
matchedData
still keepsRecord<string, any>
as the default return type.Note
While types might not be strictly accurate depending on how the generic type is created, I believe it's better than the current solution (
Record<string, any>
).As suggested in #1041 (comment), though, I believe a more type could be provided from the library depending on the options provided (e.g. with
includeOptionals: true
) via type guards. I'm open to exploring that direction further if you (maintainers) believe it might be worth implementing 馃憤To-do list