Skip to content
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

Conversation

otorrillas
Copy link
Contributor

@otorrillas otorrillas commented Sep 20, 2023

Description

As brought up in #1041, it's currently hard to type the result of parsedData. It comes with all fields set as any unless we do a type assertion via the as keyword.

const { something } = parsedData(req);
// something has any type

const { something } = parsedData(req) as { something: string };
// something has string type

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:

function matchedData<T = Record<string, any>>(
  req: Request,
  options: Partial<MatchedDataOptions> = {},
): T {
	// ...
}

Allowing users to keep using matchedData with the same signature as before:

const { something } = parsedData(req);
// something has any type

but to provide a type signature of the expected return (if it's been validated) if they'd like:

const { something } = parsedData<{ something: string }>(req);
// something has string type

There are no breaking changes, since matchedData still keeps Record<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

  • I have added tests for what I changed.
  • This pull request is ready to merge.

Copy link
Contributor

@arafatkn arafatkn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.

docs/api/misc.md Outdated Show resolved Hide resolved
Copy link
Member

@gustavohenke gustavohenke left a 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();
Copy link
Member

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.

@gustavohenke gustavohenke merged commit b098980 into express-validator:master Oct 16, 2023
7 of 10 checks passed
@otorrillas otorrillas deleted the oriol.torrillas/allow-match-data-generics branch October 23, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants