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

Remote validator. #127

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Remote validator. #127

wants to merge 7 commits into from

Conversation

piotrkowalczuk
Copy link
Contributor

New option that gives posibility to validate form by external application eg by http request to backend server.

@ljharb
Copy link
Collaborator

ljharb commented May 17, 2014

Can you elaborate a bit on this one? Remote validations aren't generally possible, because they're async, and form validation usually needs to happen synchronously.

Adding the possibility for an async validation seems like a pretty big feature, so I really want to make sure I understand the use cases first.

@@ -41,12 +43,29 @@ exports.create = function (fields, opts) {
a[k] = b.fields[k].data;
return a;
}, {});
b.remoteValidatorCallback = function (response) {
var form = b;
for (var fieldName in form.fields) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't ever use loops :-) Object.keys(form.fields).forEach(function (fieldName) { … }) would be much better.

@ljharb ljharb self-assigned this May 17, 2014
@piotrkowalczuk
Copy link
Contributor Author

I use this library in my project and i like it. Our backend architecture has changed. Only one who knows something about validation is backend server, but the frontend server render a html.

Its not a problem that validation is async. I corrected the code and updated the tests

callback(err, form);
};
})(b));
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why would you want/need to bypass default validators when setting the remote one? If you don't want local validators, you wouldn't set any

@artnez
Copy link
Contributor

artnez commented May 24, 2014

This should be implemented as a form validation hook. All you're doing is calling an additional async function on form validation success and assuming a very specific interface for the response.

Also calling it a "remote" validator is unnecessarily specific. What makes this "remote"?

I suggest an alternate implementation that would be more generalizable.

Created a validators array on the unbound form object (unbound_form.validators). This would be an array of validation callbacks that are invoked when bound_form.validate() is called. By default, there would be one validation callback which mimics the current behavior.

So bound_form.validate would invoke every callback in unbound_form.validators in an async series.

@piotrkowalczuk
Copy link
Contributor Author

Whats your opinion now?

@artnez
Copy link
Contributor

artnez commented May 25, 2014

Cool! Extensible form validators like this will be very useful.

One issue though. IMO the async validator should be dropped entirely. All form validators are async, so the name async is redundant (remote isn't appropriate either due to reasons mentioned above). Everyone is going to handle backend responses differently anyway. The current implementation of the validator assumes everyone handles remote errors like you do.

If there did exist stock form validators, they should conform to some common interfaces used by frameworks like Rails or whatever.

@@ -369,4 +407,3 @@ containing a HTML representation of the field.
[7]: https://nodei.co/npm/forms.png?downloads=true&stars=true
[8]: https://npmjs.org/package/forms
[9]: http://vb.teelaun.ch/caolan/forms.svg

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make sure all files have a trailing newline.

@ilkkao
Copy link

ilkkao commented Jul 27, 2014

I was little surprised that inside my custom validator I can't make async redis call to check if the account name user is proposing is reserved already and call callback() based on the result inside the redis callback. I think this is a very common use case. I'm not even sure what could be a workaround.

@piotrkowalczuk your PR has some extra files and unnecessary white space fixes. Maybe just create minimal PR that supports async custom validators.

@ilkkao
Copy link

ilkkao commented Jul 27, 2014

There was a solution. I make async validation now separately and set form.fields.account.error if there's an error before re-rendering the form. It'd still be nice to get a validator based solution if possible.

@@ -0,0 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please globally gitignore IDE-specific project directories, and please rebase this one out of this PR.

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.

4 participants