Skip to content
This repository has been archived by the owner on Sep 16, 2023. It is now read-only.

Only the last argument of a name in a CallExpression is transformed correctly. #91

Closed
mthadley opened this issue Jul 13, 2016 · 9 comments
Labels

Comments

@mthadley
Copy link
Contributor

Starting with this:

import {noop, range} from 'lodash';

function myFunc() {}

myFunc(range, noop);
myFunc(noop, noop);
myFunc(range, noop, noop);
myFunc(range, noop, range, noop);
myFunc(range, noop, range, noop, range);

It will be transformed to this:

'use strict';

var _range2 = require('lodash/range');

var _range3 = _interopRequireDefault(_range2);

var _noop2 = require('lodash/noop');

var _noop3 = _interopRequireDefault(_noop2);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

function myFunc() {}

myFunc(_range3.default, _noop3.default);
myFunc(_noop, _noop3.default);
myFunc(_range3.default, _noop, _noop3.default);
myFunc(_range, _noop, _range3.default, _noop3.default);
myFunc(_range, _noop, _range, _noop3.default, _range3.default);

I'm not sure where the issue is here on this one.

@jdalton
Copy link
Member

jdalton commented Jul 13, 2016

It looks like it's an issue with identifiers repeated as different args.

@mthadley
Copy link
Contributor Author

Also, looks like #90 might solve this?

@jdalton
Copy link
Member

jdalton commented Jul 13, 2016

Ya, I'll hold off digging into this until that one is merged.

The issue still exists after the merge of #90.

@jdalton jdalton added the bug label Jul 13, 2016
@jridgewell
Copy link
Contributor

Interesting. It's definitely finding and updating the references (noop gets changed to _noop), but Babel isn't finding each of them and converting it to the import reference _noop3.default. This is directly related to injecting a new import, and probably an upstream issue.

@jdalton
Copy link
Member

jdalton commented Jul 13, 2016

Yep, I can confirm it's finding all the references in binding.referencePaths but it seems babel isn't recognizing the changes too all of the paths.

_.each(binding.referencePaths, path => {
  path.replaceWith(identifier);
})

\cc @hzoo

@jdalton
Copy link
Member

jdalton commented Jul 14, 2016

@jdalton
Copy link
Member

jdalton commented Jul 15, 2016

Circling back...

The issue appears to be a Babel cache of some kind. We memoize and reuse our identifier objects. However, as this issue shows, it can lead to path.replaceWith ignoring repeat identifier replacements. The solution at the moment looks like shallow cloning the identifier object to avoid the Babel cache.

@jridgewell
Copy link
Contributor

Nice find! types provides a #clone for nodes, instead of using lodash's.

@jdalton
Copy link
Member

jdalton commented Jul 15, 2016

🙌

I'm not sure all the places that may need the clone, but we'll start out with replaceWith calls and add as needed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

3 participants