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: remove the module system detection from punycode.js #7401

Closed
yorkie opened this issue Jun 24, 2016 · 8 comments
Closed

punycode: remove the module system detection from punycode.js #7401

yorkie opened this issue Jun 24, 2016 · 8 comments
Labels
punycode Issues and PRs related to the punycode module bundled in Node.js.

Comments

@yorkie
Copy link
Contributor

yorkie commented Jun 24, 2016

  • Version: latest
  • Platform: any
  • Subsystem: punycode

lib/punycode.js looks more like the dependency required by the core module url, which is a third party library, and the current source file lib/punycode.js is not that same style with other source files.

So I propose the following:

  • add a deps directory in ./lib directory, so we can put the punycode.js there and load it just like what we are doing except for the source path. (Recommend)
  • update the punycode source file to be the CMD way just like other .js files, this also needs us to add tests and maintainer for this little module even though we are just moving them from the punycode.js project.
@yorkie
Copy link
Contributor Author

yorkie commented Jun 24, 2016

Sorry, make a noise issue because of bad networking, closed.

@yorkie yorkie closed this as completed Jun 24, 2016
@Fishrock123 Fishrock123 reopened this Jun 24, 2016
@Fishrock123
Copy link
Contributor

I closed the other. :)

@yorkie
Copy link
Contributor Author

yorkie commented Jun 24, 2016

Ah Thank you @Fishrock123

@yorkie
Copy link
Contributor Author

yorkie commented Jun 24, 2016

cc @mathiasbynens, the author of the current punycode.js :-)

@silverwind
Copy link
Contributor

Why not move it to deps? That way, we could also remove the exception in .eslintignore. I'm not oposed to rewriting (and reducing) the code in there itself, but we have to keep in mind that updating it will be a lot less straight-forward.

@yorkie
Copy link
Contributor Author

yorkie commented Jun 26, 2016

Not putting it into deps just because it's a pure library in pure JavaScript, so it should be loaded in runtime, not the compiling time. This is that the differences.

@targos targos added the punycode Issues and PRs related to the punycode module bundled in Node.js. label Jul 13, 2016
@jasnell
Copy link
Member

jasnell commented Aug 1, 2016

Recommend closing this given the decision to deprecate the punycode module bundled in core. See: #7941

@yorkie
Copy link
Contributor Author

yorkie commented Aug 1, 2016

Closing this as @jasnell's recommendation :-)

@yorkie yorkie closed this as completed Aug 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
punycode Issues and PRs related to the punycode module bundled in Node.js.
Projects
None yet
Development

No branches or pull requests

5 participants