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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.custom() not working with promise #444

Closed
ghost opened this issue Oct 4, 2017 · 6 comments
Closed

.custom() not working with promise #444

ghost opened this issue Oct 4, 2017 · 6 comments
Labels

Comments

@ghost
Copy link

ghost commented Oct 4, 2017

I am trying to get .custom() working from the check API but it appears that it's not working?

Working (only showing essential information):

check.body('email').trim().isEmail().normalizeEmail().custom((_value) => {
    return true;
});

expressValidatorCheck.validationResult(req)..throw();

Not working (only showing essential information:

check.body('email').trim().isEmail().normalizeEmail().custom((_value) => {
    return Promise.resolve();
});

check.validationResult(req).throw();

The docs state:
The custom validator may return a promise to indicate an async validation task. In case it's rejected, the field is considered invalid.
Therefore I would expect both examples to result in a "passed validation" however the second example always fails.

@gustavohenke
Copy link
Member

Thanks for the report @jplusje.

The code currently expects all validators to unbox as a truthy value. This means even async validators that return promises should have a value, eg Promise.resolve(true) to be considered as passed.

@ghost
Copy link
Author

ghost commented Oct 5, 2017

After changing my code to always resolve the promise but include a true/false it indeed works. I've also tested it with my actual implementation which returns a real promise and resolves it later and that also works.

thanks!

@gustavohenke
Copy link
Member

Yes, but as the docs say, a resolved promise should mean a passed validation, and this is not happening yet. Will fix it so this doesn't happen to other users in the future.

@dimpen
Copy link

dimpen commented Nov 18, 2017

This happened to me too. It needs to resolve(true). You have to mention it in the API documentation until you change it otherwise it's very confusing. BTW I don't think it's a bug IF you mention it in the docs.

@gustavohenke
Copy link
Member

@dimpen that's not the intended behaviour.

@lock
Copy link

lock bot commented May 31, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants