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

expanded array loses variable reference #4

Closed
gajus opened this issue Jun 14, 2019 · 4 comments
Closed

expanded array loses variable reference #4

gajus opened this issue Jun 14, 2019 · 4 comments
Labels
bug Something isn't working

Comments

@gajus
Copy link

gajus commented Jun 14, 2019

import {
  map
} from 'inline-loops.macro';

map([], ([a, b]) => {
  console.log(a, b);
});

becomes:

const _iterable = [];
let _result = [];

for (let _key = 0, _length = _iterable.length, _value; _key < _length; ++_key) {
  _value = _iterable[_key];
  _result[_key] = console.log(a, b);
}

_result;

a and b are undefined.

@gajus gajus changed the title expanded array looses variable reference expanded array loses variable reference Jun 14, 2019
@planttheidea
Copy link
Owner

Ah yes ... this is a current limitation, destructuring of the values in the callback is not supported with the aggressive inliner, and that should be checked against. I'll update.

@planttheidea planttheidea added the bug Something isn't working label Jun 15, 2019
@planttheidea
Copy link
Owner

Alright I just coupled this fix with the release of 1.1.0. The above result now will transpile into:

const _iterable = [];

const _fn = ([a, b]) => {
  console.log(a, b);
};

let _result = [];

for (let _key = 0, _length = _iterable.length, _value; _key < _length; ++_key) {
  _value = _iterable[_key];
  _result[_key] = _fn(_value, _key, _iterable);
}

_result;

Thanks for the issue with the precise repro, made this an easy bug to squash. If anything else comes up, let me know!

@gajus
Copy link
Author

gajus commented Jun 19, 2019

I am thinking if this is the best solution. This had a slight performance disadvantage. It would be nice to have a configurable warning about this. When warning is enabled, then it should suggest to use alternative syntax without destructuring to avoid the penalty of function invocation.

Perhaps this would be better as an ESLint rule, though.

@planttheidea
Copy link
Owner

planttheidea commented Jun 19, 2019

This has actually been upgraded as of 1.2.0! We inline the operation almost all the time now, including in destructured param scenarios. You can check the README for the known bailout scenarios.

That said, your callout of providing a warning in the (now rare) bailout scenario is a good one. I'll make a note to add that in very soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants