Duplicating lines of code

[Calidus]

I am testing out ts-migrate-full on a large project that uses an older version of react. ts-migrate (maybe the explicit-any plugin??) doesn't seem to be properly replacing code in some plain .js files, as you can see in the code snippet below.

Before

var window = {
    onResetData: function() {
      this.clearNextPush = function() {
        this.resetHistoryResumeData();
        this.setState({ history: [] });
        pubSub.trigger(events.window.CLEAR_HISTORY);
      };
    },
    // more code here
}

After

var window = {
    onResetData: function() {
    (this as any).clearNextPush = function () {
        this.resetHistoryResumeData();
        this.setState({ history: [] });
        pubSub.trigger(events.window.CLEAR_HISTORY);
    };
        (this as any).setState({ history: [] });
        pubSub.trigger(events.window.CLEAR_HISTORY);
      };
    },
    // more code here

}

Expected

var window = {
    onResetData: function() {
    (this as any).clearNextPush = function () {
        this.resetHistoryResumeData();
        (this as any).setState({ history: [] });
        pubSub.trigger(events.window.CLEAR_HISTORY);
      };
    },
    // more code here
}

I am seeing ~1000 errors from tsc but its really just ~200 instances this thought the code base. Manually fixing these bugs isn't that bad, when compared to all the other work ts-migrate did for me.

[zb140]

This may be related to an issue I'm seeing with the add-conversions plugin. Here's an example:

const foo = {
    func: function () {
        Object.values({ })
            .filter((x) => x.prop)
            .forEach((x) => {
                const y = x.prop;
            });
    },
};

This transforms into this code, with similar duplication:

const foo = {
    func: function () {
        Object.values({})
    .filter((x) => (x as $TSFixImplicitAny).prop)
    .forEach((x) => {
    // @ts-expect-error ts-migrate(2571) FIXME: Object is of type 'unknown'.
    const y = x.prop;
});
                // @ts-expect-error ts-migrate(2304) FIXME: Cannot find name 'x'.
                const y = (x as $TSFixImplicitAny).prop;
            });
    },
};

There are a few interesting things I've figured out while trying to debug this. One, in order for this code to transform incorrectly, you need both the filter and the forEach call. If you remove either one of them, the transform is fine. Secondly, if you modify the filter call in the original version to use braces, like this:

// [...]
.filter(x => { return x.prop; })
// [...]

the transform will be successful without any duplicated lines.

My current theory is that the replaceNode function here needs some extra smarts to be able to deal with arrow functions, but there may be more to it than that. Playing around with some modifications to that function does certainly change exactly which parts and how much of the file gets duplicated, but I'm pretty new to the world of this kind of transformation, and I'm not sure I understand how it works well enough yet to come up with an actual fix.

[haldunanil]

Can confirm @zb140 's assessment that this issue seems to be happening with the add-conversions plugin. It doesn't seem to be limited to filter and forEach calls for me however; more specifically, it seems to happen when the following are true:

• anonymous function that is passed as parameter to another function call
• function call has child(ren) that need(s) to be converted to as any

For example, let's say we have code as follows:

constants.DEPARTMENT.OPTIONS = _.sortBy(
  Object.keys(constants.DEPARTMENT.ROLE_MAP).map((value) => {
    return {
      value,
      text: constants.DEPARTMENT.ROLE_MAP[value],
    };
  }),
  ["text"]
);

where constants is an object that has no defined shape via types/interfaces. Running ts-migrate as is will produce the following, where a duplication occurred when the as any was trying to be added:

(constants as any).DEPARTMENT.OPTIONS = _.sortBy(Object.keys((constants as any).DEPARTMENT.ROLE_MAP).map((value) => {
    return {
        value,
        // @ts-expect-error ts-migrate(2339) FIXME: Property 'DEPARTMENT' does not exist on type '{}'.
        text: constants.DEPARTMENT.ROLE_MAP[value],
    };
}), ["text"]);
    return {
    value,
    text: (constants as any).DEPARTMENT.ROLE_MAP[value],
};
  }),
  ["text"]
);

However, if this were to be fixed ahead of time to:

constants.DEPARTMENT.OPTIONS = _.sortBy(
  Object.keys(constants.DEPARTMENT.ROLE_MAP).map((value) => {
    return {
      value,
      text: (constants as any).DEPARTMENT.ROLE_MAP[value],
    };
  }),
  ["text"]
);

then the duplication will NOT happen. One workaround that seems to work is extracting the callback function and making it a named function outside like so:

const innerFn = (value) => {
    return {
      value,
      text: constants.DEPARTMENT.ROLE_MAP[value],
    };
  }

constants.DEPARTMENT.OPTIONS = _.sortBy(
  Object.keys(constants.DEPARTMENT.ROLE_MAP).map(innerFn),
  ["text"]
);

Then running ts-migrate succeeds, with the output looking like so:

const innerFn = (value) => {
  return {
    value,
    text: (constants as any).DEPARTMENT.ROLE_MAP[value],
  };
};

(constants as any).DEPARTMENT.OPTIONS = _.sortBy(
  Object.keys((constants as any).DEPARTMENT.ROLE_MAP).map(innerFn),
  ["text"]
);

Though this is a bit too heavy handed a change for my taste.

[zanatoshi]

Hello

I am currently migrating a old react project to typescript. The tool is awesome but I have a lot of this duplicate lines. Any news on this subject ?

[Calidus]

I have been poking around at this, and think its either replacement ordering problem or changes aren't getting propagated up the node tree correctly.

replaceNode will first create the update for (this as any).setState({ history: [] }); then it will create a update for (this as any).clearNextPush = function() { this.setState({ history: [] }); }. The second update doesn't include the changes of the first update.

Test Code

var window = {
  onResetData: function() {
    this.clearNextPush = function() {
      this.setState({ history: [] });
    };
  },
}

[Calidus]

Looking at updateSourceText.ts leads me to believe the updates shouldn't be overlapping because it simply applies the changes to the source text sequentially. There is even a todo in verifyUpdates function that mentions this. @zb140 and @haldunanil mentioned the replaceNode logic but now I am wonder if its the shouldReplace function that needs some extra smarts because we need to make sure we not generate a second overlapping update.

[Rudeg]

Should be fixed with #181

[shawngustaw]

@Rudeg could you publish a new version to npm? Would be super grateful if so :)

[Rudeg]

@shawngustaw sorry for the delay, done

[doncollins]

As of 0.1.32, I continue to get mangled output in certain cases, e.g. running this through add-conversions:

function foo() {
  return {}.prop;
}

export default {}.prop;

produces:

function foo() {
    return {}.prop;
}
export default ({} as any).prop;


  return ({} as any).prop;
}

export default {}.prop;

[Calidus]

There are most likely a few other ancestor node types that we need to check for when deciding if the node should be replaced. I don't know of a good way to actually figure out what those are other than people reporting issues.

Hopefully it is as simple as just adding another test and additional ts.SyntaxKind check.

[Calidus]

@doncollins Sadly its not simple as just adding Block to the ancestor node check. While it fixes the duplication in the example above, it also adds an extra trailing \n character and causes other tests to fail.