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

Fix, replace for..of on for loop #2096

Merged

Conversation

NikitaDudin
Copy link
Contributor

@NikitaDudin NikitaDudin commented Jun 2, 2021

Description

Application crash when using transform or color properties on a style object. Replacing for..of loop on for loop solves it problem.

Issue

@piaskowyk
Copy link
Member

Could you provide an example code to reproduce this issue?

@NikitaDudin
Copy link
Contributor Author

@piaskowyk #1879 (comment)

@mrousavy
Copy link
Contributor

mrousavy commented Jun 6, 2021

Looks like the JS runtime (or the babel plugin) doesn't whitelist the iterator as a native prop. for..of uses [Symbol.iterator], no?

Maybe is Iterator.next() not whitelisted and REA thinks that function does not exist in the UI JS Runtime, which it does.

@piaskowyk piaskowyk self-requested a review June 8, 2021 20:10
@piaskowyk piaskowyk self-assigned this Jun 8, 2021
Copy link
Member

@piaskowyk piaskowyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for PR 🙌
Could you just change i += 1 to ++i?

@piaskowyk
Copy link
Member

piaskowyk commented Jun 9, 2021

@NikitaDudin which version of babel and Hermes do you use?

@NikitaDudin
Copy link
Contributor Author

@piaskowyk hermes: 0.7.2 babel: 7.9.0

@NikitaDudin
Copy link
Contributor Author

NikitaDudin commented Jun 9, 2021

Thanks for PR 🙌
Could you just change i += 1 to ++i?

Are you sure me should change to the prefix form ++i? May be i++?

@piaskowyk
Copy link
Member

Thanks for PR 🙌
Could you just change i += 1 to ++i?

Are you sure me should change to the prefix form ++i? May be i++?

Yes, ++i is a little faster than i++ and i++ is faster than i += 1 😅. Sure it can depend on the runtime version, but general idea is that. I think that every above expression is the same readable, but if we can choose faster - why not - what do you think? 🤔

@piaskowyk
Copy link
Member

piaskowyk commented Jun 9, 2021

I generally don't pick on things like that because this doesn't make a big difference, but I optimized this part of the code last time and I want to make it as fast as possible. 😅

@piaskowyk
Copy link
Member

@NikitaDudin good job, thanks 💪

@piaskowyk piaskowyk merged commit 3c2bf7c into software-mansion:master Jun 9, 2021
@NikitaDudin NikitaDudin deleted the replace-for-of-on-for branch June 10, 2021 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants