-
Notifications
You must be signed in to change notification settings - Fork 223
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
Incorrect Transformation of Ternary Expressions Containing Function Calls #208
Comments
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>
);
}; |
Seems related to #144 |
@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! |
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: |
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:
|
@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! |
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!! |
No problem! Glad I was able to help some. This is the list of plugins I ended up going with:
Looks like I disabled |
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. |
@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 |
@CodySchaaf @GitMurf have you tried to use the same typescript version |
@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.
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 :-( |
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. |
@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. |
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
Expected Output
Actual Output
Steps to Reproduce
Additional Information
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
Possible Solution
It does seem like this will solve it
But I'm not comfortable enough with the package to know if this could have any negative consequences.
The text was updated successfully, but these errors were encountered: