Codecov Report

Merging #280 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
+ Coverage   99.56%   99.56%   +<.01%     
==========================================
  Files          83       83              
  Lines        1145     1151       +6     
==========================================
+ Hits         1140     1146       +6     
  Misses          5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fefe390...aa7531c. Read the comment docs.

Hi @SindreSvendby, thanks for your efforts.

Unfortunately the fix for this isn't really quite as simple. Here's a test that will fail as a result of your changes:

it('should transform the same param in multiple functions', () => {
  expectTransform(
    'function fn1(a) {\n' +
    '  console.log(a.foo);\n' +
    '}\n' +
    'function fn2(b) {\n' +
    '  console.log(b.foo);\n' +
    '}'
  ).toReturn(
    'function fn1({foo}) {\n' +
    '  console.log(foo);\n' +
    '}\n' +
    'function fn2({foo}) {\n' +
    '  console.log(foo);\n' +
    '}'
  );
});

This also partly explains why there aren't any issues labeled as good first issues - I've fixed all the simple things already. All the remaining issues are really not that simple.

This also partly explains why there aren't any issues labeled as good first issues - I've fixed all the simple things already. All the remaining issues are really not that simple.

That sounds a bit discouraging for someone who wants to contribute/help. Sorry, but It sounded like "all issues left here are complicated, nothing simple. If you don't do complicated leave"

Anyhow, still a good try @SindreSvendby Kudos!

Thx for the extra test, will see if I can find a way to support that also.

I can like a good challenge.

I was hoping to get a bit of guidance on the best challenges that are not monsters, the class transform seems to have multiple issues, but I'm not sure if that is an indication that it's a good place to start, or if the entire transform should be rewritten.

Maybe a better question is, what should I stay away from 😄

I think more people will be willing to try if all the things marked limitation and bugs are marked as issues with test on them, most already have issues, so it's a good start

Ok, spent two more minutes on this.

So as far as I can see, a good implement here, would be a recursive inScope function that keep tracks on the identifiers declared in the scope. I naively thought that the was what withScope was dooing.
It seems like a general purpose helper that could be reused more places, but I'm way to new to this project to know if that in fact is the case.

Please let me know if you think I'm going down the wrong rabbit hole here!

Variables is not hoist and it's lexical scope, so should be able to identify the scope and it's identifiers, if not I guess I will learn something!

Currently the fundamental problem is that while this withScope() helper gives us scope information about existing variables (so we can avoid conflicts when introducing new ones), but the information will not be updated and as soon as we introduce a new variable, things will get out of sync.

One way to overcome this problem would be to re-generate the scoping information each time a new variable is introduced. That would be tricky to do while in the middle of processing the arguments of one function - but I think for the single-function case we can implement a simple local cache of introduced variable names.

So, my suggestion would be to first solve the problem of handling the duplicates problem in this case:

function fn(a, b) {
  console.log(a.foo, b.foo);
}

And then afterwards look into the harder problem of fixing the other test case.

BTW. You're right about the class transform - it does need a rewrite to properly fix majority of its problems.

I looked through other issues, and found two possibly simpler issues to tackle:

• Proper handling of super() call semantics #186 super() calls semantics. While it is part of the class transform, this part can be worth implementing separately (we can likely carry that logic easily over when rewriting the class transform on one fine day).
• new function to class constructor #266 new function edge case. Pretty odd corner case, but would be nice if we handled it.

One possibility is to look into issues labeled as "blocked externally" to see if they're still blocked.

I'm closing down this old PR.