haraka / node-address-rfc2821

RFC2821 Email Address parser (from Haraka)
https://www.npmjs.com/package/address-rfc2821
13 stars 6 forks source link

Idn support #6

Closed baudehlo closed 7 years ago

baudehlo commented 7 years ago

Should facilitate SMTPUTF8 support in Haraka

codecov-io commented 7 years ago

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%) :arrow_up:

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 commented 7 years ago

TODO

baudehlo commented 7 years ago

FWIW punycode is core node.js

On Mon, May 22, 2017 at 7:41 PM, Matt Simerson notifications@github.com wrote:

TODO

  • add punycode as package dependency
  • bump version in package.json
  • update Changes.md
  • publish to npm

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/haraka/node-address-rfc2821/pull/6#issuecomment-303249325, or mute the thread https://github.com/notifications/unsubscribe-auth/AAobY-e4nGjYB9zoTr9ukMNmpkS_-kFiks5r8h0hgaJpZM4Ni_1R .

msimerson commented 7 years ago

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 commented 7 years ago

Didn't know that. Fair enough.

baudehlo commented 7 years ago

What typo?

msimerson commented 7 years ago

To find the typo, search for adress

andris9 commented 7 years ago

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 commented 7 years ago

We'll just ignore the keyword.

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

msimerson commented 7 years ago

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 commented 7 years ago

OK. This should be ready to merge and release now.

On Thu, May 25, 2017 at 1:13 PM, Matt Simerson notifications@github.com wrote:

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.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/haraka/node-address-rfc2821/pull/6#issuecomment-304066795, or mute the thread https://github.com/notifications/unsubscribe-auth/AAobYx_99b3HqkI6uQfrVTF2SNRhXeC8ks5r9bangaJpZM4Ni_1R .

msimerson commented 7 years ago

Also, there's a conversation regarding punycode over at https://github.com/nodejs/node/pull/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.

baudehlo commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

$ npm publish
+ address-rfc2821@1.1.0