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

Validation phase of normalizeEmail lacks flexibility #723

Closed
noinkling opened this issue Sep 12, 2017 · 2 comments · Fixed by #725
Closed

Validation phase of normalizeEmail lacks flexibility #723

noinkling opened this issue Sep 12, 2017 · 2 comments · Fixed by #725

Comments

@noinkling
Copy link

noinkling commented Sep 12, 2017

normalizeEmail currently starts by calling isEmail. This might make sense as a guard, but there is currently no way to pass in options, which may result in false negatives for people who make use of the options, particularly require_tld. Additionally it's simply not obvious that normalizeEmail also validates unless you look at the code; this might be resulting in people doing unnecessary double validations.

I can see a few ways to solve this:

  • Allow passing in additional options for the isEmail call, and clearly document that this is a "2-in-1" function.

  • Pass in the most permissive set of options, i.e. { allow_display_name: true, require_display_name: false, allow_utf8_local_part: true, require_tld: false }, and let the user handle any more restrictive validation beforehand.

  • Remove the isEmail call altogether and let the user handle any validation beforehand. Or provide an option to do so. This is my preference because I think that normalizeEmail should only be responsible for one thing.

Display name support might need to be added if it's desired.

Actually I ran across this because I'm primarily using Joi for validation in a Node app (just because it is schema-based and has structured errors built-in), but I'm also using this library for the email normalization (among other things). I found email addresses like "whatever@local" that passed with Joi's defaults (refer to this) weren't passing normalizeEmail's check. isEmail has the require_tld: false option that would fix this, but there's no way to pass it through. As a workaround, Joi has a similar option minDomainAtoms which can be set to 2 to better match this library's default behavior, but it's still a workaround... as a general principle I'd rather just not have (restrictive) validation unless I ask for it.

@linkinmedo
Copy link
Contributor

I agree with you that option 3 is the logical one, if @chriso agrees I'll work on it.

@chriso
Copy link
Collaborator

chriso commented Sep 12, 2017

Yeah agreed, we should remove validation and add a caveat to the README that says that the result is undefined unless the input is an email.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants