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

punycode: named anonymous functions #21761

Closed
wants to merge 1 commit into from

Conversation

venetah
Copy link

@venetah venetah commented Jul 11, 2018

punycode: named anonymous functions

Refs: #8913

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The punycode module is a legacy third-party vendored-in module that is currently deprecated and only used when ICU is not present or someone is specifically calling require('punycode'). I don't believe we should be making any changes to this module, even if largely cosmetic.

@Trott
Copy link
Member

Trott commented Jul 11, 2018

@jasnell It looks like we use punycode 2.0.0 but 2.1.1 is the most recent. Would you be open to the idea that a non-breaking update to 2.1.1 is appropriate? (Trying to redirect @venetah to a different good first contribution. Although I wonder if there would be lint failures...but first thing's first. Is it even an option in your view?)

@jasnell
Copy link
Member

jasnell commented Jul 11, 2018

I'm not opposed to updating version of the module, but any changes to the module itself should be done upstream

@Trott
Copy link
Member

Trott commented Jul 11, 2018

FWIW, we did change four lines of the module in 2e568d9. But that can be floated again if necessary. (I would say it's not necessary. Simply don't land the change on v6.x and it should be fine, especially for something legacy/deprecated.)

Looks like there are no lint errors either... EDIT: ...because punycode is in the eslintignore file...

@Trott
Copy link
Member

Trott commented Jul 11, 2018

@venetah How would you feel about closing this PR and trying again with a punycode.js upgrade PR? I think you could do it this way (using UNIX-y commands, you will need to modify if you are on Windows):

  • create a new branch starting with the current master branch
  • grab version 2.1.1 of punycode.js from somewhere. This is one way:
$ mkdir tmp-dir && cd tmp-dir && npm install punycode@2.1.1
...
$ cp node_modules/punycode/punycode.js PATH_TO_punycode.js_IN_NODE

(You can remove tmp-dir after that.)

  • Build with the new punycode with make -j4.
  • Run tests with make test
  • If everything looks good, open a pull request.

/cc @trivikr

@Trott Trott mentioned this pull request Jul 11, 2018
2 tasks
@BridgeAR
Copy link
Member

I am going to close this due to the mentioned reasons.

@venetah thanks a lot for your contribution! It is highly appreciated. Please also feel encouraged to follow up on the suggestion from @Trott

@BridgeAR BridgeAR closed this Jul 12, 2018
@Trott
Copy link
Member

Trott commented Jul 15, 2018

@venetah I've opened a pull request for the punycode update. If this is something you were planning on doing, let me know and I'll close mine or at least marked it as blocked pending yours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants