punycode: deprecate punycode module #7552

jasnell · 2016-07-06T00:09:20Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

punycode

Description of change

Currently, the punycode module is used in exactly one spot within core (the url parser). With the recent switch to using the much faster ICU based punycode implementation by default, the punycode module is now only used when node happens to be built without ICU. This change moves the punycode module to internal/punycode and hard deprecates require('punycode').

When the new WHATWG URL implementation lands, users will have access to the URL.domainToUnicode() and URL.domainToASCII statics that are defined as part of the standard interface.

The next step (in the next major) is to remove require('punycode') entirely and make it so that internal/punycode.js is only included if the Node.js binary is built without ICU.

Why? - (a) we do not maintain or support the punycode module, (b) it's 10x slower than the ICU based implementation, (c) we use it exactly once currently, (d) there are userland options, (e) deprecating and eventually removing reduces overall core API surface area.

jasnell · 2016-07-06T00:22:41Z

@ChALkeR ... would you be able to do a search to see how much of any impact this would have on the ecosystem?

jasnell · 2016-07-06T15:11:38Z

@nodejs/ctc

silverwind · 2016-07-06T16:49:25Z

lib/punycode.js

-
-module.exports = punycode;
+const util = require('internal/util');
+util.printDeprecationMessage('The punycode module in core is deprecated.');


Should we suggest the npm package?

silverwind · 2016-07-06T16:51:33Z

LGTM with suggestion.

Fishrock123 · 2016-07-06T16:57:15Z

Can we expose this functionality to ICU in a way that isn't hugely breaking? That would seem to be the best route as this functionality is useful, and having it from ICU without a native add-on could be a plus.

bnoordhuis · 2016-07-06T17:15:04Z

LGTM. I think @silverwind's suggestion is a good one.

trevnorris · 2016-07-06T18:27:33Z

I assume that URL.domainTo*() will be available before punycode is removed?

rvagg · 2016-07-07T12:06:15Z

lgtm, one less thing to expose

ChALkeR · 2016-07-07T12:39:09Z

@jasnell Will check a bit later today.

jasnell · 2016-07-07T13:44:10Z

That's the intent, yes. I already have both implemented in my whatwg URL PR.
On Jul 6, 2016 11:27 AM, "Trevor Norris" notifications@github.com wrote:

I assume that URL.domainTo*() will be available before punycode is
removed?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7552 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eeUXcX1jwvydtrNaFV0DA7voIiAgks5qS_OcgaJpZM4JFpPr
.

Fishrock123 · 2016-07-07T14:19:16Z

I'd still like to know why we don't just continue exposing it but with an ICU-backed implementation?

... May I remind people that we can't even get rid of sys? it should surely go before this.

jasnell · 2016-07-07T17:11:07Z

@Fishrock123 ... we could replace parts of it, yes. There are a couple of extraneous functions exposed that we'd have to reimplement if we were to go that route. Just don't see the point tho, for those people who happen to use the punycode module, they can easily keep on using it from npm.

If it makes things easier, we can do a soft-deprecation in v7 and only do the hard-deprecation in v8.

Fishrock123 · 2016-07-07T17:13:55Z

for those people who happen to use the punycode module, they can easily keep on using it from npm.

You don't even need to use npm to use the util module.

Sorry, I don't buy that we can just get rid of the module unless we have clear data that says literally no-one uses it.

cc @ChALkeR I suppose.

jasnell · 2016-07-07T17:35:10Z

See: #7552 (comment) and #7552 (comment)

Note that this PR does not remove the punycode module at this time. It simply deprecates it for v7

jasnell · 2016-07-25T13:53:43Z

ping @nodejs/ctc

jasnell · 2016-07-25T13:54:20Z

Putting on the ctc agenda to see if we can get resolution.

ChALkeR · 2016-07-25T13:54:30Z

Argh, totally forgot about this, sorry. Will post some stats today =).

bnoordhuis · 2016-07-25T14:05:52Z

For the record, while the PR technically LGTM last time I looked at it, the punycode module itself never bothered me. It does the one thing it does well and we hardly ever get bug reports for it.

Replacing it with a ICU-backed implementation is kind of meh, IMO. That would break something that isn't broken in non-ICU builds.

jasnell · 2016-07-25T14:13:07Z

In non-ICU builds, the punycode module is still used so nothing breaks there. I could make it so that the hard deprecation notice is not printed in non-ICU builds if that would be better.

Currently, the punycode module is used in exactly one spot within core (the url parser). With the recent switch to using the much faster ICU based punycode implementation by default, the punycode module is now only used when node happens to be built without icu. This change moves the punycode module into internal and hard deprecates `require('punycode')`. The hard deprecation notice is only printed in ICU builds. When the new WHATWG URL implementation lands, users will have access to the URL.domainToUnicode() and URL.domainToASCII statics that are defined as part of the standard interface. The next step (in the next major) is to make it so that internal/punycode.js is only included if the Node.js binary is built without ICU.

jasnell · 2016-07-25T14:35:36Z

Updated the PR so that the deprecation notice is only printed if the build includes ICU, this way non-ICU builds are not affected.

ChALkeR · 2016-07-25T14:51:21Z

@jasnell — https://gist.github.com/ChALkeR/6dc79b79918f7728659fbc28c5723961. That's a bit old, though, I could make the new dataset today or tomorrow if needed.

jasnell · 2016-07-25T15:12:51Z

Quite a few hits... thank you @ChALkeR ...
@nodejs/ctc ... given the number of hits, it would make sense that if we're going to deprecate, we do so first with a soft deprecation first in v7 then look at a hard deprecation later in v8.

ChALkeR · 2016-07-27T20:32:55Z

Note that there is a punycode package on the npm. Not sure if it's compatible, but if it is, it could simplify things.

Upd: I somehow missed that this was already mentioned above =).

Upd2: b77eb8c — both internal and npm module seem to be identical, v2.0.0.

jasnell · 2016-07-27T20:34:24Z

The punycode module on npm is an updated version of what we ship in core. It's API is compatible as far as I can tell.

srl295 · 2016-07-27T20:38:55Z

the punycode module on npm is by (and CC:) @mathiasbynens

ChALkeR · 2016-07-27T20:42:23Z

@jasnell Btw, the punycode module has 5 736 456 downloads per month. This could indicate that most of that require('punycode') are actually already using the module, or this could indicate that a lot of people are using both of those =).

Btw, punycode is marked as being Stability: 2 - Stable. So +1 on soft deprecation first.

jasnell · 2016-07-27T20:45:55Z

Discussion by the CTC would be to soft deprecate in v7 and revisit hard deprecation prior to v8.

yorkie · 2016-07-28T11:10:35Z

@jasnell this might close #7401 as a side-effect :-)

jasnell · 2016-08-01T18:57:57Z

Closing this PR in favor of #7941
Hard-deprecation of the punycode module will be revisited post v7

As discussed and agreed upon by the CTC, the punycode module bundled in core is soft-deprecated (docs only) for v7 with an eye towards hard-deprecation in v8 or later. Also see discussion in #7552 PR-URL: #7941 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Ref. nodejs/node#7941.

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 6, 2016

nodejs-github-bot added url Issues and PRs related to the legacy built-in url module. build Issues and PRs related to build files or the CI. punycode Issues and PRs related to the punycode module bundled in Node.js. labels Jul 6, 2016

jasnell changed the title ~~punycode: deprecate internal punycode module~~ punycode: deprecate punycode module Jul 6, 2016

jasnell force-pushed the deprecate-punycode branch from 86e752b to 27cfae2 Compare July 6, 2016 00:09

mscdex removed the build Issues and PRs related to build files or the CI. label Jul 6, 2016

silverwind reviewed Jul 6, 2016
View reviewed changes

jasnell force-pushed the deprecate-punycode branch from 27cfae2 to 6a03329 Compare July 7, 2016 17:36

jasnell added the ctc-agenda label Jul 25, 2016

jasnell force-pushed the deprecate-punycode branch from 6a03329 to 7b0c497 Compare July 25, 2016 14:34

Trott mentioned this pull request Jul 26, 2016

Node.js Foundation Core Technical Committee (CTC) Meeting 2016-07-27 #7881

Closed

srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Jul 27, 2016

jasnell mentioned this pull request Aug 1, 2016

doc, punycode: soft-deprecation of the punycode module #7941

Closed

2 tasks

jasnell closed this Aug 1, 2016

mathiasbynens referenced this pull request in mathiasbynens/punycode.js Aug 11, 2016

README: Note that Punycode.js won’t ship with Node.js forever

a086555

Ref. nodejs/node#7941.

Trott removed the ctc-agenda label Oct 20, 2016

msimerson mentioned this pull request May 25, 2017

Idn support haraka/node-address-rfc2821#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

punycode: deprecate punycode module #7552

punycode: deprecate punycode module #7552

jasnell commented Jul 6, 2016 •

edited

Loading

jasnell commented Jul 6, 2016

jasnell commented Jul 6, 2016

silverwind Jul 6, 2016

silverwind commented Jul 6, 2016

Fishrock123 commented Jul 6, 2016

bnoordhuis commented Jul 6, 2016

trevnorris commented Jul 6, 2016

rvagg commented Jul 7, 2016

ChALkeR commented Jul 7, 2016

jasnell commented Jul 7, 2016

Fishrock123 commented Jul 7, 2016

jasnell commented Jul 7, 2016

Fishrock123 commented Jul 7, 2016 •

edited

Loading

jasnell commented Jul 7, 2016

jasnell commented Jul 25, 2016

jasnell commented Jul 25, 2016

ChALkeR commented Jul 25, 2016

bnoordhuis commented Jul 25, 2016

jasnell commented Jul 25, 2016

jasnell commented Jul 25, 2016

ChALkeR commented Jul 25, 2016

jasnell commented Jul 25, 2016

ChALkeR commented Jul 27, 2016 •

edited

Loading

jasnell commented Jul 27, 2016

srl295 commented Jul 27, 2016

ChALkeR commented Jul 27, 2016 •

edited

Loading

jasnell commented Jul 27, 2016

yorkie commented Jul 28, 2016

jasnell commented Aug 1, 2016

punycode: deprecate punycode module #7552

punycode: deprecate punycode module #7552

Conversation

jasnell commented Jul 6, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

jasnell commented Jul 6, 2016

jasnell commented Jul 6, 2016

silverwind Jul 6, 2016

Choose a reason for hiding this comment

silverwind commented Jul 6, 2016

Fishrock123 commented Jul 6, 2016

bnoordhuis commented Jul 6, 2016

trevnorris commented Jul 6, 2016

rvagg commented Jul 7, 2016

ChALkeR commented Jul 7, 2016

jasnell commented Jul 7, 2016

Fishrock123 commented Jul 7, 2016

jasnell commented Jul 7, 2016

Fishrock123 commented Jul 7, 2016 • edited Loading

jasnell commented Jul 7, 2016

jasnell commented Jul 25, 2016

jasnell commented Jul 25, 2016

ChALkeR commented Jul 25, 2016

bnoordhuis commented Jul 25, 2016

jasnell commented Jul 25, 2016

jasnell commented Jul 25, 2016

ChALkeR commented Jul 25, 2016

jasnell commented Jul 25, 2016

ChALkeR commented Jul 27, 2016 • edited Loading

jasnell commented Jul 27, 2016

srl295 commented Jul 27, 2016

ChALkeR commented Jul 27, 2016 • edited Loading

jasnell commented Jul 27, 2016

yorkie commented Jul 28, 2016

jasnell commented Aug 1, 2016

jasnell commented Jul 6, 2016 •

edited

Loading

Fishrock123 commented Jul 7, 2016 •

edited

Loading

ChALkeR commented Jul 27, 2016 •

edited

Loading

ChALkeR commented Jul 27, 2016 •

edited

Loading