mathiasbynens / punycode.js

A robust Punycode converter that fully complies to RFC 3492 and RFC 5891.
https://mths.be/punycode
MIT License
1.6k stars 159 forks source link

Replace "for of" loops with simple for loop to remove polyfill requirement #74

Closed kaushlakers closed 6 years ago

kaushlakers commented 6 years ago

Hey, just wanted to say I love this lib. It's lightweight and efficient.

We are using it in twitter-text for url validation.

There are a few usages of "for of" loop in the implementation, that is slightly problematic for legacy browsers. This is transpiled down to a variant that uses "Symbol.Iterator" by babel. Unfortunately "Symbol" is also not supported by IE11 and needs to polyfilled.

I noticed that all the usages iterate over an array and could be replaced by a for loop. I understand that this is not the responsibility of punycode, but I think it'd be pretty useful if we remove a polyfill dependency for legacy browsers.

Also added the package-lock that comes with npm5 to the gitignore.

mathiasbynens commented 6 years ago

From the README:

The current version supports recent versions of Node.js only. It provides a CommonJS module and an ES6 module. For the old version that offers the same functionality with broader support, including Rhino, Ringo, Narwhal, and web browsers, see v1.4.1.

mathiasbynens commented 6 years ago

Is there a reason you’re not just using that version?

kaushlakers commented 6 years ago

Ah I see. Looks like I missed it. I'll go ahead and lock the version, thanks!

Any reason you prefer to maintain 2 versions like this? I didn't see use of any javascript language features present in recent versions of node other than the "for of" loop. Just curious if there are other gains here I'm missing by using an older version 😃

rcastera commented 6 years ago

@mathiasbynens @jdalton @nathanhammond @mscdex ^

jdalton commented 6 years ago

Hi @kaushlakers!

I am not a fan of that heavier babel transpiled form either so I use babel-plugin-transform-for-of-as-array:

plugins: [
  "transform-for-of-as-array"
],
presets: [
  ["@babel/env", {
    exclude: [
      "transform-for-of"
    ],
    loose: true
  }]
]

It should work great with punycode, I had punycode in my bundle as well.

Update:

The babel-plugin-transform-for-of-as-array plugin was just updated with a loose mode to allow nullish values.

Update:

It looks like Babel ported babel-plugin-transform-for-of-as-array to an official plugin: PR: https://github.com/babel/babel/pull/6914; Docs babel/babel/packages/babel-plugin-transform-for-of

You can use the assumesArray options

"plugins": [
  ["@babel/transform-for-of", {
    "assumeArray": true
  }]
]