Remove env detection code in lib/punycode.js

[stevemao]

I realize some modules like readable-stream is maintained internally but punycode is maintained externally. Should we maintain them internally only? Reasons are:

1. Consistency.
2. https://github.com/nodejs/node/blob/master/lib/punycode.js#L5-L16 exports the module depends on the environment which is not needed here in node.

[rvagg]

What are you actually suggesting here @stevemao? I'm not sure what the question is that is needing an answer. Is this specifically about punycode or are there other files in scope? Are you suggesting maintaining a fork of punycode and not accepting upstream updates?
If this is only about punycode then I'd say I don't see a compelling argument for detaching from upstream. The expertise required for implementing and maintaining this module is pretty specialist and I for one am happy to lean on the punycode project to deal with that. Perhaps you could outline more about why you think this is so compelling?

[stevemao]

OK, I changed my mind @rvagg :) I only think that the environment detection code should be removed and just keep module.exports. We don't need amd and global (window) support here in the core.

We should of course accept upstream updates, etc..

[Fishrock123]

Isn't punycode meant to work in the browser? We just copy it verbatim, so it won't make a difference?

[stevemao]

Correct, just to make it a bit smaller @Fishrock123 . No other code in the /lib folder has those detection. Even though it's copied from other source, The code could be modified to suit node core better.

[Fishrock123]

I'm not sure it would be worth it still, might as well just keep it so you can copy from the upstream.

[ChALkeR]

I think that there are two viable solutions here:

1. Add a floating patch that removes the unneeded env detection.
2. Just leave it as it is.

I doubt that maintaining an interfnal fork will do any good.

[ChALkeR]

Btw, v1.4.0 is out, it might be time to update: https://github.com/bestiejs/punycode.js/releases
Edit: the changes look comment-only.

[cjihrig]

Here is the diff for v1.4.0 - cjihrig@e55844b

[Fishrock123]

Is it possible we updated from upstream before the release?

[cjihrig]

It looks that way. Do you see any value with brining it up to date with 1.4.0, even though it would just be comment changes?

[ChALkeR]

@Fishrock123 There are no actual changes between those two versions, except for this pull: mathiasbynens/punycode.js#32, which is the mirror of #1246.

Diff: mathiasbynens/punycode.js@v1.3.2...v1.4.0#diff-c50a9dbe44909e8f5b09f11d33fe6f1c.

That's why I said above that the changes are comment-only.

[stevemao]

Can we change the label? This is not a question but a minor change request :)

[stevemao]

created PR #4969

[evanlucas]

As stated in the PR, this is an upstream module that we use. Making this change will make updating it more difficult. Thanks!