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

Idn support #6

Merged
merged 7 commits into from
May 25, 2017
Merged

Idn support #6

merged 7 commits into from
May 25, 2017

Conversation

baudehlo
Copy link
Contributor

Should facilitate SMTPUTF8 support in Haraka

Matt Sergeant added 2 commits May 22, 2017 19:04
Not ready yet - need to provide the punycode if the domain is utf8
@codecov-io
Copy link

codecov-io commented May 22, 2017

Codecov Report

Merging #6 into master will increase coverage by 1.07%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
+ Coverage   84.81%   85.88%   +1.07%     
==========================================
  Files           1        2       +1     
  Lines          79       85       +6     
  Branches       16       17       +1     
==========================================
+ Hits           67       73       +6     
  Misses         12       12
Impacted Files Coverage Δ
_idn.js 100% <100%> (ø)
index.js 85.71% <93.75%> (+0.9%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91c6b00...18a482c. Read the comment docs.

@msimerson
Copy link
Member

msimerson commented May 22, 2017

TODO

  • fix typo (see comment)
  • add punycode as package dependency
  • bump version in package.json
  • update Changes.md
  • after Travis CI passes tests
    • merge PR
    • publish to npm

@baudehlo
Copy link
Contributor Author

baudehlo commented May 23, 2017 via email

@msimerson
Copy link
Member

https://nodejs.org/api/punycode.html

"The version of the punycode module bundled in Node.js is being deprecated. In a future major version of Node.js this module will be removed. Users currently depending on the punycode module should switch to using the userland-provided Punycode.js module instead."

(We already explicitly depend on the npm packaged version of punycode in haraka-tld).

@baudehlo
Copy link
Contributor Author

baudehlo commented May 23, 2017 via email

@baudehlo
Copy link
Contributor Author

What typo?

Matt Sergeant and others added 2 commits May 23, 2017 13:53
@msimerson
Copy link
Member

To find the typo, search for adress

@andris9
Copy link

andris9 commented May 24, 2017

Regarding punycode, I included punycode.js in Nodemailer for a while but stopped doing that pretty fast and kept using the deprecated native module:

  1. If you use the latest stable of punycode.js then it is going to start showing warnings on older versions of Node (including v4) because punycode.js requires at least Node v6
  2. Whichever version of punycode.js you are using, Node is not going to use it. If there are multiple modules with the same name, then Node prefers the built-in one and there already is a module named punycode

So what happened in my case was that people started to complain about the warnings when installing Nodemailer even though the module that caused the warnings wasn't actually even used.

Regarding RFC6531, do you intend to use the SMTPUTF8 keyword for MAIL FROM somehow or do you just silently ignore it? If it is just ignored then I guess it doesn't really matter though.

@baudehlo
Copy link
Contributor Author

We'll just ignore the keyword.

@msimerson what do you think about the npm module issue?

@msimerson
Copy link
Member

what do you think about the npm module issue?

Punycode was deprecated in v7 and is expected to be removed in node v8. I'd much rather add the explicit dependency and so we're future-ready than worry about users of node v4 getting a module warning. Users getting this newer version of Haraka and module will be mostly new installs and most distros are already bundling node v6 or v7. I suspect warnings on node v4 will be few and will tail off to zero over the next 10 months when node v4 is EOL.

@baudehlo
Copy link
Contributor Author

baudehlo commented May 25, 2017 via email

@msimerson
Copy link
Member

Also, there's a conversation regarding punycode over at nodejs/node#7552. As of node 7.4 there's experimental domainToASCII and domainToUnicode functions in the url module. So perhaps in node v8+, we'll no longer use the punycode module at all (see that conversation in 7522 thread). In the meantime, an explicit declaration will work.

@msimerson msimerson merged commit 2df7e36 into master May 25, 2017
@msimerson msimerson deleted the idn_support branch May 25, 2017 17:21
@baudehlo
Copy link
Contributor Author

I'm having trouble publishing this - it seems to be published by "haraka" but not one of @haraka the group's npm modules: https://www.npmjs.com/org/haraka

Do we need to email npm support?

@msimerson
Copy link
Member

msimerson commented May 26, 2017

Unfortunately, @haraka (groups) don't work the way you'd expect on NPM. I just manually added your and Steve's npm ids to this module.

@msimerson
Copy link
Member

I think you'd be able to had you tried again but I just published this.

$ npm publish
+ address-rfc2821@1.1.0

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