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

- use simple for loop instead of forEach #677

Conversation

knalbandianbrightgrove
Copy link

Babel is not transforming on runtime such things like forEach which is critical when a product going to be used as SaaS solution.
I have everything to transpile ES6 code, but some specific methods make me sad.
I would appreciate if you approve this small change

Thank you in advance!

@coveralls
Copy link

coveralls commented Jan 14, 2019

Coverage Status

Coverage increased (+0.05%) to 87.739% when pulling 88584c3 on knalbandianbrightgrove:feature/support-old-browsers-without-polyfills into 5c7c61d on visionmedia:master.

@Qix-
Copy link
Member

Qix- commented Jan 14, 2019

.forEach has been around since ES5. Your loop is also incredibly inefficient, pulling Object.keys() each iteration.

There is no reason why this should be merged, nor should the existing code be causing you problems. If Babel is transpiling this incorrectly, please open an issue with them.

@Qix- Qix- closed this Jan 14, 2019
@Qix- Qix- added the invalid This issue is out of scope or has been deemed invalid label Jan 14, 2019
@knalbandianbrightgrove
Copy link
Author

knalbandianbrightgrove commented Jan 14, 2019

@Qix-
Thank you for the quick response.

I made a typo when talking about ES6, originally I mean ES5.

Babel is not transpiling .forEach, it should be polyfilled. So in order to prevent extra polyfilled

One more argument is that in this file you already using the same idea, please take a look
https://github.com/knalbandianbrightgrove/debug/blob/88584c3ecbcee0447e4ff37018085106ec34ef2b/src/common.js#L184

@Qix-
Copy link
Member

Qix- commented Jan 14, 2019

It's not transpiling down to es3? What ancient browser are you targeting?

@knalbandianbrightgrove
Copy link
Author

@Qix- it does transpiling everything but not methods, in order to work with such things like Array.prorotype.forEach you should use babel-polyfill. I need to support IE8+.
I do not see any performance downgrade in switching to for loop if you care about performance

@knalbandianbrightgrove
Copy link
Author

@Qix- sorry for disturbing you.

Any logic reason to not approve this PR?

@Qix-
Copy link
Member

Qix- commented Jan 15, 2019

Babel 7 includes support for this, your PR is for compatibility reasons that are caused by either user error or a problem with Babel. It doesn't make any improvement to me. Please open an issue with Babel if polyfilling isn't working as expected.

I do not wish to support ES3.

@knalbandianbrightgrove
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This issue is out of scope or has been deemed invalid
Development

Successfully merging this pull request may close these issues.

None yet

3 participants