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

check: support default options for validationResult #474

Conversation

alrav
Copy link
Contributor

@alrav alrav commented Nov 20, 2017

This is a proposal to resolve #427, by introducing a way to set default options for the validation-result module

It introduces a new .withDefaults function, which will return a new instance of the module with specified default options. The decision to return a new instance was to avoid affecting the global module state in the case that multiple validationResult implementations are desired within the same application.

This is loosely inspired on the concept of request.defaults from the request/request module

Here's a brief example:

// baseValidationResult.js ----
const { validationResult } = require('express-validator/check');

const result = validationResult.withDefaults({
    formatter: (error) => {
        return {
            myLocation: error.location,
        };
    }
});

module.exports = result;

// within an express router ----
const validationResult = require('./baseValidationResult');

errors = validationResult(req);

if (!errors.isEmpty()) {
  return res.status(422).json({
    errors: errors.mapped(),
  });
}

Note: This is my first time getting acquainted with TypeScript, so let me know if the definitions make sense

@coveralls
Copy link

coveralls commented Nov 20, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 68c949f on alrav:issue-427-validation-result-defaults into ead6b75 on ctavan:master.

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.

This is perfect - thanks!!
Exactly how I would approach that issue.

I left some comments regarding minor things though.
Can you please fix them?

README.md Outdated
@@ -367,6 +367,28 @@ try {
}
```

### `.withDefaults(options)`
- `options` *(optional)*: an object of options. Defaults to `{ formatter: error => error }`
> *Returns:* a new validation result instance with provided default options
Copy link
Member

Choose a reason for hiding this comment

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

Saying "a new validation result instance" could be misleading.
What do you think of rewriting it to say that a new validationResult function is returned, using the provided options? Will also be great to link to that function's section in the docs!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes perfect sense to me

@@ -23,6 +24,15 @@ result.formatWith(error => {
error.value;
});

withDefaults({
Copy link
Member

Choose a reason for hiding this comment

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

Can you use validationResult.withDefaults()? So that it matches the documented example.
I think this will also require changes to how the type declaration is done...

Copy link
Member

Choose a reason for hiding this comment

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

It will also be worth changing the test to const result: ValidationResult = validationResult.withDefaults(...)(req).
Should catch all possible typing bugs...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be set now.

I kept validationResult interface as lower-camel-cased, since it might be more easily identifiable as a function. If any objections, I can also change to ValidationResult.

@alrav
Copy link
Contributor Author

alrav commented Nov 23, 2017

Thanks for the feedback and absolutely! I'm working on addressing your requested changes 👍

@coveralls
Copy link

coveralls commented Nov 23, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling de770e2 on alrav:issue-427-validation-result-defaults into ead6b75 on ctavan:master.

@alrav
Copy link
Contributor Author

alrav commented Nov 23, 2017

Hello @gustavohenke - comments addressed. Let me know if anything else needs a tweak.

@alrav alrav force-pushed the issue-427-validation-result-defaults branch from de770e2 to 8c6245e Compare November 23, 2017 06:27
@coveralls
Copy link

coveralls commented Nov 23, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 8c6245e on alrav:issue-427-validation-result-defaults into ead6b75 on ctavan:master.

@gustavohenke
Copy link
Member

Thanks again!

@gustavohenke gustavohenke merged commit 85d1f8f into express-validator:master Nov 25, 2017
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global errorFormatter not used
3 participants