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

Restructure API #4

Open
skeggse opened this issue Sep 14, 2015 · 15 comments · May be fixed by #200
Open

Restructure API #4

skeggse opened this issue Sep 14, 2015 · 15 comments · May be fixed by #200
Assignees
Milestone

Comments

@skeggse
Copy link
Owner

skeggse commented Sep 14, 2015

I'm not fond of the current API usage, especially the errorLevel option. It would also be nice to support email address normalization (removing comments and condensing other particles). What should the API look like?

Ideas:

  • IsEmail.check('test@hapijs.com', {dns: true|false}, callback)
  • IsEmail.normalize('test(for testing)@hapijs.com') -> 'test@hapijs.com'
  • IsEmail.parse('test(for testing)@hapijs.com') -> {local: 'test', domain: 'hapijs.com'}
@bf
Copy link

bf commented Jun 3, 2016

Also it'd be great if the callback would conform to the (err, isValid) style. Right now it only uses callback in a callback(isValid) fashion.

@nonplus
Copy link

nonplus commented Jun 9, 2016

+1 on having the callback follow node conventions function(err, result)

FYI, this is how I've normalized the API for my usage to make it Node-friendly:

/**
 * Validate email address for syntax and (optionally) valid MX record
 *
 * @param {String} email Email address to check
 *
 * @param {Object} [options] Configuration options
 * @param {Boolean} [options.dns=false] check DNS for MX record
 * @param {Number} [options.errorLevel=0] strictness of email syntax checking
 *
 * @param {Function} [callback] Callback function
 * @param {Error} callback.err Error if validation fails, otherwise null
 * @param {Number} callback.result Diagnostic code
 */
function validateEmail(email, options, callback) {
    options = options || {};

    if (typeof options === 'function') {
        callback = options;
        options = {};
    }

    if (typeof callback !== 'function') {
        if (options.checkDNS) {
            throw new TypeError('expected callback function for checkDNS option');
        }

        callback = null;
    }

    // Convert options to format expected by isemail.validate()
    var opts = {
        checkDNS: options.dns,
        errorLevel: true
    };

    return isemail.validate(email, opts, function(result) {
        if (callback) {
            if (result <= (options.errorLevel||0)) {
                callback(null, result);
            } else {
                callback(new Error("Invalid email"), result);
            }
        }
    });
}

@enko
Copy link

enko commented Jan 31, 2018

Also supporting promises would be nice if checkDNS is set and no callback is specified.

Loopback handles this quite nice:

https://github.com/strongloop/loopback/blob/master/common/models/role.js#L238
https://github.com/strongloop/loopback/blob/master/lib/utils.js#L16

@skeggse
Copy link
Owner Author

skeggse commented Jan 31, 2018

FYI, we no longer implement checkDNS in isemail.

That use-case will be filled via the parse method, whereby users of the library can run their own DNS query against the identified, normalized domain.

@alexsasharegan
Copy link
Contributor

Some typings would be very appreciated as well! I'm working on annotating the current api to make my project happy, I would be happy to PR my definitions.

I'm reading that you're interested in updating the API. If you're open to typescript, perhaps it would be nice to open a PR on a dev branch just defining a TS interface? Not everyone's favorite, but could be a nice way to define the API changes before implementation work.

@skeggse
Copy link
Owner Author

skeggse commented Feb 1, 2018

I have no idea what that would entail - is it just the addition of a types file? By open dev branch do you mean somehow modifying branch-level permissions? I'd be fine supporting typescript, but I don't have any bandwidth to do so myself.

@alexsasharegan
Copy link
Contributor

alexsasharegan commented Feb 1, 2018

Well then let's just stick to adding typings to the existing API. The addition is a single index.d.ts file along with a package.json addition for typings.

I think I've got the API documented. PR #164 is up, @skeggse.

@hueniverse
Copy link
Contributor

Is this still happening?

@skeggse
Copy link
Owner Author

skeggse commented Nov 11, 2018

I made some headway within the last month. If I can block out another weekend, I'll have this done. That'll enable folks a clean way to test address validity with DNS checks.

@hueniverse
Copy link
Contributor

Is there an api to validate just the domain part of the email?

@skeggse
Copy link
Owner Author

skeggse commented Nov 11, 2018

Not currently - the API I described is mostly done and just exposes the parsed components of the email address. Either the user or another library could wrap isemail and do a DNS check on the parsed domain part.

@hueniverse
Copy link
Contributor

No I mean, use the parser to validate a domain, not an email.

@skeggse
Copy link
Owner Author

skeggse commented Nov 11, 2018

I wasn't planning on it - isemail's domain validation mostly handles the strange syntacies of email domains and domain literals. Is that a use-case you want? Maybe we should move this to a new issue if so.

@hueniverse
Copy link
Contributor

I think it would be useful to be able to validate domains in joi instead of implementing it all over again. Is there a reason why an email domain would require different validation than any other FQDN?

@skeggse
Copy link
Owner Author

skeggse commented Nov 19, 2018

It's mostly that we support domain literals like eran@[127.0.0.1] which isn't useful for domain validation. That said, it could be useful to consolidate unicode-aware domain validation.

The only concern there is that I don't have the bandwidth to mitigate IDN homograph attacks and track the current recommendations. I can design the API around that limitation, though. I'll have more time to think about this in a couple weeks.

@skeggse skeggse linked a pull request Feb 8, 2019 that will close this issue
@skeggse skeggse added this to the 4.0.0 milestone Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@enko @hueniverse @bf @nonplus @skeggse @alexsasharegan and others