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

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

Conversation

kaushlakers
Copy link

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
Copy link
Owner

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
Copy link
Owner

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

@kaushlakers
Copy link
Author

kaushlakers commented Jan 30, 2018

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
Copy link

@jdalton
Copy link
Contributor

jdalton commented Jan 31, 2018

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: babel/babel#6914; Docs babel/babel/packages/babel-plugin-transform-for-of

You can use the assumesArray options

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

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