search

mike-lischke / antlr4ng

Next Generation TypeScript runtime for ANTLR4
Other
66 stars 11 forks source link

Moving all ".js" to ".ts" files with ts-migrate #3

Closed OskarDamkjaer closed 9 months ago

OskarDamkjaer commented 9 months ago

This PR renames all .js files to .ts and adds bulk //@ts-ignore and friends to all the resulting new errors. To make sure no behaviour changed I did not run any "eslint --fix" or simliar, so in my testing the bundled file has been identical to before.

The project already had typescript declaration files, and the new typescript files will take preceidence when both are present. This is problematic because the new .ts files don't always match the existing .d.ts signatures. To not make any externally visible changes I change the type declation entry point to be in the dist folder and copy the .d.ts files there on build.

The old declaration files also got overriden by the new .ts files when using .eslint, to retain the typechecking I added them as a new project in the eslint config.

I needed to make a few small changes to the tests, since they are js, they can no longer import by source code and need to import the built project instead (or be converted to typescript). Changing this led me to update a few tests because they were dependent on unminimized class names.

disclaimer

This is a really big change and I can see not wanting the project littered with "ts-ignore" style comments everywhere, so it's fine if you prefer do a more "traditional" move to typescript with allowJs and migrating on a file by file basis.

OskarDamkjaer commented 9 months ago

Just glancing over the PR myself now, I think I'd prefer migrating on a file by file basis to avoid the soup of // @ts-ignores that comes with this approach. I see you're working through configurations (like the jest/jasime) things today already @mike-lischke, so perhaps it's better to let the dust settle a little and check back in a week or so.

mike-lischke commented 9 months ago

Thanks for taking a stab at that already. Yes, I have moved to a simpler setup, which supports TS out of the box (well, Jest needs a little help from ts-jest, but that's just a matter of a bit extra config).

I agree that doing it file by file is the better choice. I started with the ATN class and just pushed a patch. Works nicely and I see no degradation in speed. I believe that's the way to go.

mike-lischke commented 9 months ago

btw. @OskarDamkjaer, don't forget to sign your patches. In case you don't know the DCO stuff look in the failed check, which contains enough information to get you going.

OskarDamkjaer commented 9 months ago

Ah so you've gotten started with the convertion already, nice! Btw, have you tried vitest for testing? It supports typescript out of the box, supports the same syntax as jest and I've found it to be much faster than jest in other projects (even when not using vite).

mike-lischke commented 9 months ago

That's on my todo list actually for that MySQL for VS Code extension (which uses vite for builds). If it turns out to be easy to handle then I might consider switching to it, but so far I'm using now Jest in like 10 projects of mine, so it's very easy for me to set it up and have debugging etc. working.

OskarDamkjaer commented 9 months ago

In my experience switching from jest to vitest has just been just removing the jest.config file and then everything works out of the box! There's no rush to switching though since you've got jest set up already.