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

New transform: for loops to for of #166

Closed
siddharthkp opened this issue Aug 15, 2016 · 6 comments
Closed

New transform: for loops to for of #166

siddharthkp opened this issue Aug 15, 2016 · 6 comments

Comments

@siddharthkp
Copy link

siddharthkp commented Aug 15, 2016

Convert this

for (var i = 0; i < fruits.length; i++) {
   console.log(fruits[i]);
}

to this

for (let fruit of fruits) {
  console.log(fruit)
}
@siddharthkp siddharthkp changed the title Transform: for loops to for of New transform: for loops to for of Aug 15, 2016
@nene
Copy link
Collaborator

nene commented Aug 16, 2016

This is definitely something that's been in the plans for a long time. Unfortunately it's not quite so simple transforms to write, so haven't really taken up it so far. There are several things to consider:

Skipping cases where array index is also used

I think the following can't be transformed:

for (var i = 0; i < fruits.length; i++) {
   console.log(fruits[i], i);
}

Naming of the loop variable

Sometimes there's a var declared inside the loop, which we could eliminate:

for (var i = 0; i < fruits.length; i++) {
   var fruitItem = fruits[i];
   console.log(fruitItem);
}
// -->
for (var fruitItem of fruits) {
   console.log(fruitItem);
}

At other times we'll need to make up the name by ourselves. A fairly good case is when the array is named in plural then we can pick a variable name that's the singular form of it:

for (var i = 0; i < fruits.length; i++) {
   console.log(fruits[i]);
}
// -->
for (var fruit of fruits) {
   console.log(fruit);
}

I'm not really sure what to do with other cases where we're looping over a variable like array. Perhaps we can add -Item suffix:

for (var i = 0; i < array.length; i++) {
   console.log(array[i]);
}
// -->
for (var arrayItem of array) {
   console.log(arrayItem);
}

But this way we might also end up with silly stuff like itemItem.

Automatically generating useful variable names is a tricky business...

Avoid naming conflicts

When we introduce a new name we'll also have to watch out for possible existing variables with the same name, like:

var fruit = "banana";
for (var i = 0; i < fruits.length; i++) {
   console.log(fruits[i]);
}

PS: And this transform should not change from var to let or const - this is the job of the let transform. It should keep the existing variable kind.

nene added a commit that referenced this issue Aug 18, 2016
@nene
Copy link
Collaborator

nene commented Aug 18, 2016

After laying out my thoughts above I realized it might not be so hard after all and did an initial implementation. Currently only supporting the case where we already have a name for loop variable.

There's also the case that variable scoping is not properly accounted for. I wonder whether it would make sense to only perform the transform when the loop uses let/const - in which case I don't need to worry about loop variables being used outside the loop. One can simply run let transform first and for-of transform afterwards.

@siddharthkp
Copy link
Author

siddharthkp commented Aug 18, 2016

@nene What is the expectation from a transform (or in general with lebab)?

Is it to convert all your code to es2015?
OR
Is to transform code and learn from the changed code?

Take this for example- Accurate singular names for fruits, array is not a trivial problem, not completely solve-able by a script. You would want a human to step in and see what would make more sense.

I see lebab as a companion (not as a tool) to convert code to es6

That being said, I'm perfectly okay, if all my code was not transformed. Pick the obvious cases and only convert those.

@nene
Copy link
Collaborator

nene commented Aug 18, 2016

@siddharthkp I think that's my general opinion as well. One really shouldn't use Lebab blindly - the changes need some human review.

Then again, Lebab should not do changes that might break stuff. Ideally when reviewing the transformed code you would not need to fix any actual bugs, but more like manually transform the cases that Lebab didn't handle or adjust the code style when Lebab didn't format the code to the best of your preferences.

In that regard some transforms are very safe, like obj-shorthand. Some can currently fail with rare edge-cases, like arrow, while the goal is to fix these bugs. But some transforms don't guarantee strictly equivalent code - like the default arguments aren't really equivalent to a = a || 3 - I've classified those to the "Unsafe" category. The latter ones can't really be implemented in a way that would make the transform perfectly safe, but they work fine for most of the code.

nene added a commit that referenced this issue Aug 18, 2016
Require that the 'let' transform is run beforehand.
This eliminates the need to do complex scope-detection
within this transform - let transform will take care of that,
and if it's unable to perform the transform, the for-of
transform can also not be applied.

Refs #166
nene added a commit that referenced this issue Aug 18, 2016
Directing users to use let transform first.

Refs #166
nene added a commit that referenced this issue Aug 19, 2016
Luckily we already have a helper for that

Refs #166
nene added a commit that referenced this issue Aug 19, 2016
@nene
Copy link
Collaborator

nene commented Aug 19, 2016

This is now released as v2.2.0

@nene
Copy link
Collaborator

nene commented Aug 19, 2016

Closing this for now, but created a follow-up for further development: #169

@nene nene closed this as completed Aug 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants