Incorrect Transformation of Ternary Expressions Containing Function Calls

[CodySchaaf]

When applying the add-conversions plugin on TypeScript code involving ternary expressions that return function expressions in one branch and property accesses in the other, the output code is malformed. Specifically, the transformation incorrectly duplicates part of the code and introduces syntax errors.

Input Code

export function Complaints({ currentUserCanEdit, updateComplaint }) {
  const complaint = {};

  return currentUserCanEdit
    ? () => {
        updateComplaint(complaint.id);
      }
    : complaint.type;
}

Expected Output

export function Complaints({ currentUserCanEdit, updateComplaint }) {
  const complaint = {};

  return currentUserCanEdit
    ? () => {
        updateComplaint((complaint as any).id);
      }
    : (complaint as any).type;
}

Actual Output

export function Complaints({ currentUserCanEdit, updateComplaint }) {
  const complaint = {};

  return currentUserCanEdit
    ? () => {
        updateComplaint(complaint.id);
    }
    : (complaint as any).type;
        updateComplaint((complaint as any).id);
      }
    : complaint.type;
}

Steps to Reproduce

1. Create a TypeScript file with the provided input code snippet.
2. Run the add-conversions plugin from the ts-migrate tool on this file.
3. Observe the malformed output as described above.

Additional Information

• The problem seems to arise from the transformation logic not correctly handling the scoping or branching of ternary expressions, especially when mixed with function expressions, which is a simplified version of a problem I ran into when converting a react component that contained a component within a ternary operation.

{currentUserCanEdit ? (
                      <Input
                        widthOptions="s"
                        onChange={(e: any) => {
                          updateComplaint(e.currentTarget.value, 'caseNumber', complaint.id, index)
                        }}
                        value={complaint.caseNumber}
                      />
                    ) : (
                      complaint.caseNumber
                    )}

Request for Guidance
I've attempted to address this issue by adding a new failing test case to highlight the problem, but I'm unsure where within the add-conversions plugin's transformation logic this case should be specifically handled. Any insights or suggestions on how to properly transform these expressions without introducing syntax errors would be greatly appreciated.

New Failing Test

it("handles ternary expressions containing a function call", async () => {
    const text = `export function Complaints({ currentUserCanEdit, updateComplaint }) {
  const complaint = {};

  return currentUserCanEdit
    ? () => {
        updateComplaint(complaint.id);
      }
    : complaint.type;
}
`;
    const result = addConversionsPlugin.run(await realPluginParams({ text }));

    expect(result)
      .toBe(`export function Complaints({ currentUserCanEdit, updateComplaint }) {
  const complaint = {};

  return currentUserCanEdit
    ? () => {
        updateComplaint((complaint as any).id);
      }
    : (complaint as any).type;
}
`);
  });

Possible Solution
It does seem like this will solve it

const ancestorChecks = [
        ts.SyntaxKind.ExpressionStatement,
        ts.SyntaxKind.ConditionalExpression,
      ];
      ancestorReplaceMap.set(
        origNode,
        ancestorShouldBeReplaced === undefined
          ? ancestorChecks.includes(origNode.kind)
          : ancestorChecks.includes(origNode.kind) || ancestorShouldBeReplaced
      );

But I'm not comfortable enough with the package to know if this could have any negative consequences.

[CodySchaaf]

Oddly enough with the above solution the test:

  it("handles ternary expressions containing a function call", async () => {
    const text = `export function Complaints({ currentUserCanEdit, updateComplaint }) {
  const complaint = {};

  return currentUserCanEdit
    ? () => {
        updateComplaint(complaint.id);
      }
    : complaint.type;
}
`;
    const result = addConversionsPlugin.run(await realPluginParams({ text }));

    expect(result)
      .toBe(`export function Complaints({ currentUserCanEdit, updateComplaint }) {
  const complaint = {};

  return currentUserCanEdit
    ? () => {
        updateComplaint((complaint as any).id);
    }
    : (complaint as any).type;
}
`);
  });

Passes along with the other tests pass, but when run via this script:

const { migrate, MigrateConfig } = require("../packages/ts-migrate-server/build/src");
const { addConversionsPlugin} = require("../packages/ts-migrate-plugins/build/src");

const path = require("path");

async function run() {
  // get input files folder
  const inputDir = path.resolve(__dirname, "../");

  // create new migration config and add ts-ignore plugin with empty options
  const config = new MigrateConfig().addPlugin(addConversionsPlugin, {});

  // run migration
  const exitCode = await migrate({
    rootDir: inputDir,
    tsConfigDir: './src',
    config,
    sources: [path.resolve(__dirname, './src/test-file.tsx')],
  });

  process.exit(exitCode.exitCode);
}

run();

I get this broken output:

export function Complaints({ currentUserCanEdit, updateComplaint }) {
  const complaint = {};

  return currentUserCanEdit
    ? () => {
        updateComplaint(complaint.id);
    }
    : (complaint as any).type;
        updateComplaint((complaint as any).id);
      }
    : complaint.type;
}

And so my actual file:

import React from "react";

export const Complaints = ({ currentUserCanEdit, updateComplaint }) => {
  const complaint = {};

  return (
    <table>
      <tbody>
        <tr key={complaint.id}>
          <td>
            {currentUserCanEdit ? (
              <input
                onChange={(e) => {
                  updateComplaint(e.currentTarget.value, complaint.id, index);
                }}
                value={complaint.number}
              />
            ) : (
              complaint.number
            )}
          </td>
        </tr>
      </tbody>
    </table>
  );
};

Gets extra crazy:

import React from "react";

export const Complaints = ({ currentUserCanEdit, updateComplaint }) => {
  const complaint = {};

  return (<table>
      <tbody>
        <tr key={(complaint as any).id}>
          <td>
            {currentUserCanEdit ? (<input onChange={(e) => {
            updateComplaint(e.currentTarget.value, complaint.id, index);
        }} value={(complaint as any).number}/>) : ((complaint as any).number)}
          </td>
        </tr>
      </tbody>
    </table>);
                  updateComplaint(e.currentTarget.value, (complaint as any).id, index);
                }}
                value={complaint.number}
              />
            ) : (
              complaint.number
            )}
          </td>
        </tr>
      </tbody>
    </table>
  );
};

[CodySchaaf]

Seems related to #144

[GitMurf]

@CodySchaaf I think we are encountering the same issue as you. Any updates, tips or solutions you came up with even if manual? Or did you just have to manually go resolve any of these errors? Thanks!

[CodySchaaf]

I tried updating the plugin but couldn't figure it out, so ended up just disabling this plugin 😢

[GitMurf]

I tried updating the plugin but couldn't figure it out, so ended up just disabling this plugin 😢

@CodySchaaf yeah I tried looking at the code and couldn't figure out how to fix it either. oh so you mean you still ran ts-migrate but just didn't use this "step" (plugin) in the process? I forgot you could do that. Is it something easy to add to the command? I am running: ts-migrate migrate --aliases tsfixme . ... is there something I can add to that to disable this step in the process? or is there some sort of config file I have to edit? Thanks for your help!

[CodySchaaf]

It appeared that the -h documentation was not correct, so I found this thread about how to use it. #166 personally I ended up running into issues and just made a script to run the migrate executable and passed in the plugins with additional options ect.

Something like:

const config = new MigrateConfig().addPlugin(plugin, options);

    // run migration
    const { exitCode } = await migrate({
      rootDir: inputDir,
      tsConfigDir: rootDir,
      sources,
      config,
    });

[GitMurf]

@CodySchaaf thank you so much! I similarly have always ran into problems trying to follow help or other examples I have seen. Appreciate the link and your example instantiating and running the plugin yourself. That worked perfect for me!

There are a couple other plugins I had trouble with like for example ts-migrate auto applying eslint fixes because that actually broke some of our code due to optional chaining which eslint thought was unnecessary but in reality was necessary but because of broken Types (the whole point in running ts-migrate) our code needed certain optional chains and nullish check stuff.

Curious if you had any similar issues with eslint type stuff like this when using ts-migrate?

Would you mind sharing the ultimate config that you ran with the plugins you did or did not run? Thanks so much!

[GitMurf]

I apologize for pestering you with questions @CodySchaaf and for using this gh issue for Q&A but unfortunately with no gh Discussion on this repo I don't really know where / how else to discuss with the community!

It shocks me that there are not other similar projects as ts-migrate that have been more recently maintained / updated. Or at least none that I have been able to find reliable. https://github.com/JoshuaKGoldberg/TypeStat is the most promising but even Josh has said it isn't ready to use yet and he has not had a chance to really work on it in the last year or so. Do you know of any other similar projects / scripts you have found successful for converting / cleaning up Type errors like ts-migrate? or a fork anyone is maintaining that is more reliable today?

My last question I promise is that I found the only successful way to run ts-migrate was to revert back to pre v5 typescript to match up with ts-migrate package.json (4.7). Did you have to do the same? Thanks again!!

[CodySchaaf]

No problem! Glad I was able to help some. This is the list of plugins I ended up going with:

const anyAlias = "$TSFixMe";
const anyFunctionAlias = "$TsFixMeFunction";

const plugins = [
  // [addConversionsPlugin, { anyAlias }, true], // Add conversions to any ($TSFixMe) in the case of type errors. [Long-Running]
  [declareMissingClassPropertiesPlugin, { anyAlias }, true], // Declare missing class properties. [Long-Running]
  // [eslintFixPlugin, {}], // Run eslint fix to fix any eslint violations that happened along the way.
  [explicitAnyPlugin, { anyAlias }, true], // Annotate variables with any ($TSFixMe) in the case of an implicit any violation. [Long-Running]
  [hoistClassStaticsPlugin, { anyAlias }], // Hoist static class members into the class body (vs. assigning them after the class definition).
  [jsDocPlugin, { anyAlias }], // Convert JSDoc @param types to TypeScript annotations.
  [memberAccessibilityPlugin, {}, true], // Add accessibility modifiers (private, protected, or public) to class members according to naming conventions. [No-Changes]
  [reactClassLifecycleMethodsPlugin, {}], // Annotate React lifecycle method types.
  [reactClassStatePlugin, { anyAlias }], // Declare React state type.
  [reactDefaultPropsPlugin, { useDefaultPropsHelper: true }], // Annotate React default props.
  [
    reactPropsPlugin,
    { shouldKeepPropTypes: false, anyFunctionAlias, anyAlias },
  ], // Convert React prop types to TypeScript type.
  [reactShapePlugin, { anyFunctionAlias, anyAlias }], // Convert prop types shapes to TypeScript type.
  [stripTSIgnorePlugin, {}], // Strip // @ts-ignore. comments
  [tsIgnorePlugin, {}, false, true], // Add // @ts-ignore comments for the remaining errors.
];

Looks like I disabled eslintFixPlugin and addConversionsPlugin

[GitMurf]

Looks like I disabled eslintFixPlugin and addConversionsPlugin

We must have been running into similar issues with the eslint stuff! glad to hear it wasn't just me. I am sure you have gone through the same classic debate as me where I have been so tempted just to strip down ts-migrate to its core functions and re-write it for my own needs but I am trying so hard not to put that burden upon myself 🤣 It is just difficult because there are so many little issues I am finding that I don't even know anymore what I can surely rely upon in the results!

My next attempt (thanks to your config help) is going to be strip down the plugins I run to see if I can replicate the core logic I need but be able to feel a bit more comfortable that it isn't doing weird things to my code that I won't figure out until a user reports something broken haha.

[GitMurf]

@CodySchaaf after running without addConversionsPlugin it appears as if basically all the changes ts-migrate did were add ts-expect-error comments. It did at least get most of the tsc errors "resolved" but it did not even add a single ts-fixme any alias. Did you encounter the same thing? is addConversions required to do things like replace ts-fixme for anys etc? Thanks!

[mariorodeghiero]

@CodySchaaf @GitMurf have you tried to use the same typescript version "4.7.2" as ts-migrate? That fixed my issues, once I was 5.0.4 I needed to downgrade the version.
I hope it can help.

[GitMurf]

@mariorodeghiero thanks for your comment. I know there was a lot of back and forth above but I ultimately found the same thing as you.

I found the only successful way to run ts-migrate was to revert back to pre v5 typescript to match up with ts-migrate package.json (4.7).

Mentioned in this earlier comment: #208 (comment)

It is a shame that such a powerful and important tool like ts-migrate is no longer maintained as the workaround of going back to 4.7.2 to run ts-migrate will not be feasible much longer with all the iterations in v5+ that TypeScript has done. I tried looking into the code earlier this year to see if it was something I was willing (or able) to try and provide some PRs to help maintain it but I quickly realized I was in over my head and the codebase was going to take too much time for me to learn without any assistance :-(

[GitMurf]

Curious @mariorodeghiero did you run into the same issues above with some of the ts-migrate plugins like the eslint fix and addConversionsPlugin? I had to disable them.

[mariorodeghiero]

@GitMurf I just started so I didn't notice the ternary expressions but I took note of it. After the downgrade I noticed the ESLINT fix works much better. I have used the same eslint, tsconfig configuration as this guy https://www.youtube.com/watch?v=y7WUsi6NeH8 but once I just started more finds will come.