Repository navigation
Remove env detection code in lib/punycode.js #4848
Description
I realize some modules like readable-stream is maintained internally but punycode is maintained externally. Should we maintain them internally only? Reasons are:
- Consistency.
- 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.
Activity
rvagg commentedon Jan 25, 2016
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 commentedon Jan 25, 2016
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..
[-]Maintenance of lib[/-][+]Remove module detection code in lib/punycode.js[/+][-]Remove module detection code in lib/punycode.js[/-][+]Remove env detection code in lib/punycode.js[/+]Fishrock123 commentedon Jan 25, 2016
Isn't punycode meant to work in the browser? We just copy it verbatim, so it won't make a difference?
stevemao commentedon Jan 25, 2016
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 commentedon Jan 25, 2016
I'm not sure it would be worth it still, might as well just keep it so you can copy from the upstream.
ChALkeR commentedon Jan 25, 2016
I think that there are two viable solutions here:
I doubt that maintaining an interfnal fork will do any good.
ChALkeR commentedon Jan 25, 2016
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 commentedon Jan 25, 2016
Here is the diff for v1.4.0 - cjihrig@e55844b
Fishrock123 commentedon Jan 25, 2016
Is it possible we updated from upstream before the release?
cjihrig commentedon Jan 25, 2016
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 commentedon Jan 25, 2016
@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 commentedon Jan 26, 2016
Can we change the label? This is not a question but a minor change request :)
stevemao commentedon Jan 30, 2016
created PR #4969
evanlucas commentedon Feb 9, 2016
As stated in the PR, this is an upstream module that we use. Making this change will make updating it more difficult. Thanks!