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

Drop old browsers and modernize code #45

Closed
benjamingr opened this issue Mar 23, 2016 · 3 comments · Fixed by #48
Closed

Drop old browsers and modernize code #45

benjamingr opened this issue Mar 23, 2016 · 3 comments · Fixed by #48

Comments

@benjamingr
Copy link

Hey, would it be possible to modernize this library a little?

  • Remove IE8 related fixes.
  • Change things like map to either for loops for performance or Array#map.
  • Remove module code detection and the IIFE and assume node and CommonJS, users are used to this and browserify (or webpack) their packages anyway in recent times (can also provide a browser build).

I can write PRs if those would be entertained.

Refs: nodejs/node#5868

@benjamingr benjamingr changed the title Drop old browsers Drop old browsers and modernize code Mar 23, 2016
@mathiasbynens
Copy link
Owner

Remove IE8 related fixes.

The only IE8-specific workaround is https://github.com/bestiejs/punycode.js/blob/0fbadd6e81f3a0ce06c38998040d6db6bdfbc5c9/punycode.js#L106-L110 which doesn’t really increase code complexity or negatively impact maintainability IMHO. Do you see any other browser-specific code?

Change things like map to either for loops for performance or Array#map.

I’m open to a patch that increases map performance.

Remove module code detection and the IIFE and assume node and CommonJS, users are used to this and browserify (or webpack) their packages anyway in recent times (can also provide a browser build).

I’m okay with simplifying the module detection to just Node/CommonJS and browsers (i.e. drop Rhino, Ringo, Narwhal) for the next major release.

@benjamingr
Copy link
Author

benjamingr commented May 31, 2016

The only IE8-specific workaround is […]

Well, there are a lot of places that use map instead of the built in map that is not available in IE8.

I’m open to a patch that increases map performance.

It's probably better to use a for loop in favor of map because it's generally faster in engines. This is more of a philosophical question on how you see this library and how much of its educational value you're willing to sacrifice in order to get better performance.

I think that since this library runs on millions of computers and is in the NodeJS core we should consider refactoring for better performance. I'm willing to undertake such changes if you're willing to review them - I totally understand if you don't have motivation or time.

I’m okay with simplifying the module detection to just Node/CommonJS and browsers (i.e. drop Rhino, Ringo, Narwhal) for the next major release.

Great.

@mathiasbynens
Copy link
Owner

It's probably better to use a for loop in favor of map because it's generally faster in engines.

Did you mean “instead of”? map is a while loop currently: https://github.com/bestiejs/punycode.js/blob/0fbadd6e81f3a0ce06c38998040d6db6bdfbc5c9/punycode.js#L78-L85 Are you saying for is somehow faster than while?

I think that since this library runs on millions of computers and is in the NodeJS core we should consider refactoring for better performance.

We’re in agreement!

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 a pull request may close this issue.

2 participants