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

Remove dns lookup #153

Merged
merged 5 commits into from
Jun 22, 2017
Merged

Remove dns lookup #153

merged 5 commits into from
Jun 22, 2017

Conversation

WesTyler
Copy link
Contributor

@WesTyler WesTyler commented Feb 24, 2017

Per #152 : this PR is to remove the DNS lookup functionality and pull the punycode dependency back out.

[EDITED] This is as done as can be without further input from @hueniverse on the punycode dependency question.

@WesTyler
Copy link
Contributor Author

@skeggse - just wanted to touch base on the punycode length calculation.
I'm still hacking away at it, but it's looking more and more like I have to go through almost all of the encoding algorithm in order to determine the encoded length. I'm happy to do that implementation, but first I wanted to touch base on if that is still preferable to bringing in the Node-backed punycode dependency.

@skeggse
Copy link
Owner

skeggse commented Feb 28, 2017

Oops, sorry. I'm busy over the next week and don't have time to help with this. I'm not sure where to go with punycode in that case. @hueniverse, I get the desire to avoid extra dependencies, but this is the recommended module to replace the one in Node's core. Thoughts?

@WesTyler
Copy link
Contributor Author

WesTyler commented Mar 3, 2017

Yeah, I'm throwing in the towel on calculating a punycoded string length without implementing the full punycode algorithm. With the bias adaptation and state machine advancement, I don't think you can shortcut to the process...

Here's the 34 page RFC describing the algo and implementation if anyone else wants to take a crack at it.

@WesTyler
Copy link
Contributor Author

WesTyler commented Apr 14, 2017

Okay, in the interest of potentially getting this merged in while removing a dependency, I copied over the punycode encoding algorithm from https://github.com/bestiejs/punycode.js.

I believe this is done, but please let me know if I've missed any test cases. I updated existing Unicode tests surrounding domain length and domain label length to respect the punycode algorithm length result per RFC 5890 section 2.3.2.1, and added a new case or two to check the failing punycoded length.

@WesTyler
Copy link
Contributor Author

@skeggse @hueniverse sorry, but I'm gonna bump this.

Currently the main blocker to publishing international characters and removing DNS checks is the outstanding question around the punycode dependency. After spending a lot of time on this, I see two options available:

  1. Allow the punycode package dependency.
    - Pros: maintained, sanctioned by Node.js itself.
    - Cons: changes need to be reviewed for each update.
  2. (currently in place in this PR) Copy over the punycode algorithm code from the package.
    - Pros: no added dependency that needs to be reviewed.
    - Cons: The complex algorithm now needs to be maintained manually inside of isemail if issues ever arise.

What are your thoughts? Is there a 3rd option I neglected?

@AdriVanHoudt
Copy link

I would go with 1.
Seeing the previous releases it has around 3 releases a year (with very minor updates, v2 was just dropping old env support) which seems reasonable to review think and does seem more manageable than the possible issues isemail will get.

@WesTyler
Copy link
Contributor Author

Yeah, 1 is definitely my preference, too. I don't know that we want to be in the business of punycode maintenance, haha.

@hueniverse
Copy link
Contributor

If @Marsup agrees with my plan here: hapijs/joi#1199, you can do whatever you want here (e.g. new deps) because I will block isemail from hapi core. Since hapi core doesn't use the email() joi rule, it will not require this module. But to use it in your code, you will need to require joi yourself in your project which will bypass the shrinkwrap file in hapi. So I think we can pull this off without breaking anything.

@WesTyler
Copy link
Contributor Author

Thank you for weighing in and offering a solution, @hueniverse !

Since the Joi PR was merged in to make this an optional dependency, I went ahead and updated this PR to utilize the punycode module for checking domain lengths.

@skeggse - as far as I can tell this is now done and just awaiting your approval/feedback. :)
There are updated tests around domains that fail out on punycode length, and I believe I have updated the existing tests to have the correct error codes and/or "valid" designations as a result of this update.

@WesTyler
Copy link
Contributor Author

@skeggse any thoughts on how we should proceed here?

@skeggse skeggse merged commit ac9acbd into skeggse:master Jun 22, 2017
@skeggse
Copy link
Owner

skeggse commented Jun 22, 2017

Yeesh I knew I was behind on things but February?

skeggse pushed a commit that referenced this pull request Jun 22, 2017
This was left over from some tests introduced in #151 (0f795c5) and removed in #153 (ac9acbd).
tombh added a commit to tombh/joi that referenced this pull request Aug 24, 2017
The change in ismemail from `v2.x.x` to `v3.x.x` was a siginificant change in that it deprecated the requirement for the `dns` module: skeggse/isemail#153 Therefore `joi` requires a minor version increment.
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 this pull request may close these issues.

None yet

4 participants