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 Object.keys instead of for ... in, because with Opal. #1797

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

takaokouji
Copy link

Proposed Changes

use Object.keys instead of for ... in in Blocks.getAllVariableAndListReferences.

Reason for Changes

If added some methods to Array.prototype by some libraries like Opal, it's failed.

Test Coverage

I checked scratch-gui with this changes. Drag and drop blocks from a sprite to another sprite, and it's done.

If added some methods to Array.prototype, failed
Blocks.getAllVariableAndListReferences. Because it use for ... in.
@takaokouji
Copy link
Author

I'm developing Smalruby 3 ( https://smalruby.jp/smalruby3-gui ) based scratch-gui. I hope it will be bridge of Ruby and Scratch. It's use Opal, so I made this PR. Please merge this.

@paulkaplan
Copy link
Contributor

Hi @takaokouji. thanks for the interest, that looks like a cool project. Could you explain a bit more about the need to move away from for/in? It is quite a common JS idiom used in many places in our codebase, and we would need to introduce a lint rule or something to prevent it from coming back.

Also sorry about the delay in reviewing, we are gearing up for the 3.0 launch and so are trying to stabilize things.

@takaokouji
Copy link
Author

takaokouji commented Dec 20, 2018

@paulkaplan

Could you explain a bit more about the need to move away from for/in?

OK. I think to move away only for Array object. Almost your projects's for ... in are for Object, it's OK. However, sometime for ... in for Array object make trouble like my case that is with Opal. Opal is to add some method Array prototype.

So, please use for ... of or Array.prototype.forEach instead of for ... in for Array object.

@takaokouji
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants