-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
Add a ParseDatePipe to the common library #12848
Comments
it would parse and validate an |
Right now on my app I am not even bothering with validating valid ISO 8601 dates, I just do @Injectable()
export class ParseDatePipe implements PipeTransform {
transform(value: (string | Date) | (() => string | Date) | undefined | null) {
if (!value) throw new BadRequestException('Date is required');
if (typeof value === 'function') {
value = value();
}
const transformedValue = new Date(value);
if (isNaN(transformedValue.getTime())) {
throw new BadRequestException('Invalid date');
}
return transformedValue;
}
} But I guess validating first ISO 8601 compliance and then parsing will be a better option for library code. Something I want to point out is that since Date is an object, if you want to accept a default value that changes over time, for example, @Query('date', new DefaultValuePipe(() => new Date()), ParseDatePipe) to indicate 'default is now', you need to be able to pass a function to ParseDatePipe. Doing |
got you but I'm not sure about changing the So it should be yet another pipe like |
I don't get what you mean. DefaultValuePipe currently supports any value, including functions. It would be the responsability of ParseDatePipe to check if the value passed is a function and consequently run it, right? In my example I am already using the library's DefaultValuePipe and it works fine. |
I mean I kind of get you but my setup of DefaultValuePipe returns function, then ParseDatePipe checks if value is function, and if it is, it executes the function. If it is not, it just takes the value and validates it. |
yeah but that's because your
|
Got you. Yeah, I agree it makes more sense to make ParseDatePipe behave like other existing parsing pipes, and where you need to have a value generated each request you can use something like a |
Is it fine if I start working on a PR for that? |
@davidgonmar I'd await for Kamil |
@davidgonmar if you don't mind on writing a PR that can be rejected, go ahead :) |
Will probably write it anyways when I have some free time to do it!:) |
While I understand the need for the |
I think it is 'common enough' (since there are other pipes for usual types like UUID, Boolean, etc(I even got confused there wasn't an included ParseDatePipe) that it might be worth it to add it, but I am not sure on the usage it would get. |
tbh I don't know if we should or not do that. I'll leave this decision up to Kamil. What I found a bit off is that we one for UUID while we don't for |
Yeah I also found it a bit off since other common 'stringifiable' types, but obviously I respect the decision:) |
I believe this pipe should be inside nestjs directly to make the set of offered pipes more complete. This is very common query argument type and it's also a built-in in Javascript so I think the case is strong enough. |
another version of this pipe which can handle both required and not rw=equired usecasesfor my use-case I needed a pipe which only take care of the required values and leave not required fields aloneimport { BadRequestException, Injectable, PipeTransform } from "@nestjs/common";
@Injectable()
export class ParseDatePipe implements PipeTransform<string | Date | undefined | null> {
constructor(private readonly required: boolean = true) { }
transform(value: string | Date | undefined | null | (() => string | Date) | undefined | null): Date {
if (!this.required && !value) {
return value as undefined | null;
}
if (!value) {
throw new BadRequestException('Date is required');
}
if (typeof value === 'function') {
value = value();
}
const transformedValue = new Date(value);
if (isNaN(transformedValue.getTime())) {
throw new BadRequestException('Invalid date');
}
return transformedValue;
}
} you can use it this way: async yourController(
@Query('fromDate', new ParseDatePipe(false)) fromDate?: string | undefined,
@Query('toDate', new ParseDatePipe(false)) toDate?: string | undefined
): Promise<YourResponseDto> {
return;
} |
Is there an existing issue that is already proposing this?
Is your feature request related to a problem? Please describe it
Currently, NestJS does not provide a ParseDatePipe in the common library.
Describe the solution you'd like
Implementing a ParseDatePipe. I am open to opening a PR with the changes!
Teachability, documentation, adoption, migration strategy
Including the new pipe in https://docs.nestjs.com/pipes#built-in-pipes.
What is the motivation / use case for changing the behavior?
I think it is common enough to want to parse dates, so including it in the common library can save some time:)
The text was updated successfully, but these errors were encountered: