[P3] Remove CoffeeScript (Decaffeinate)

[marwanhilmi]

[P3] Remove CoffeeScript (Decaffeinate)

[yoganlava]

Would slowly decaffing the files work? Next time someone tries to edit a file, they could decaf it, add their logic, test and then push. JS files should still work with the other coffee files due to the interop.

[ihgrant]

Compiling the Coffeescript seems to result in circular dependency issues, at least for the API when using the code in app, visible when you try to start a new game. I wasted a bunch of time doing things manually before deciding to try the following approach:

1. compile all coffeescript files to javascript so we can use javascript tooling
2. compile away the custom module resolution rules (require('app/common/logger') instead of require('../../common/logger') so ESLint's import/no-cycle rule can work
3. install the import/no-cycle ESLint rule and fix any cyclic dependency issues found (hopefully!)

Since the v1 coffeescript compiler didn't know about ES6 features, especially classes, there is likely a ton of cleanup to be done, and I think JSCodeshift might be helpful there.

[willroberts]

More praise for Decaffeinate here: https://benchling.engineering/from-200k-lines-of-coffeescript-to-zero-making-decaffeinate-super-stable-4de0ca68d9bc

[willroberts]

More tools and documentation:
https://github.com/decaffeinate/bulk-decaffeinate
https://github.com/decaffeinate/decaffeinate/blob/main/docs/conversion-guide.md
https://github.com/decaffeinate/decaffeinate/blob/main/docs/suggestions.md

[willroberts]

When using decaffeinate --disallow-invalid-constructors, this change is necessary to make bulk-decaffeinate check work properly.

Edit: This has been merged upstream.

[willroberts]

It may be a good idea to do this one service at a time. I ran into some issues with classes extending other classes (e.g. must use new with constructors), possibly related to decaffeinate configs.

[willroberts]

I made some progress on decaffeinating the server and worker directories (leaving only app) here: willroberts/duelyst@main...willroberts:duelyst:decaffeinate-server

It mostly works, but something is up with the user data access. After logging in, I have no faction progress, while on main I have most factions unlocked. There are no explicit errors being thrown, so I'll need to dig in a bit more.

[willroberts]

decaffeinate results in logical/structural changes to the code (necessitating a good amount of work to fix things), so I also tried writing a script (https://github.com/willroberts/duelyst/compare/main...willroberts:duelyst:decaffeinate-backend?expand=1) to do this using the CoffeeScript compiler (coffee -c) instead.

It doesn't work very well, since it produces unlinted files which also need manual changes.

Since this is a fairly complex task, I'm going to hold off on it for now while we focus on the reference deployment.

[willroberts]

I looked at this again today. The reason the decaffeinate approach struggles is that we're using CoffeeScript 1.x instead of 2.x. In order to use decaffeinate, we may want to try upgrading to [email protected] first: https://coffeescript.org/#breaking-changes

The main thing here is https://coffeescript.org/#breaking-changes-super-this

This may be tough, since there are several things which seem to rely on tweaking the object state before calling super() in order to influence constructor behavior, such as code in app/sdk/actions and server/lib/custom_errors.coffee.

We might want to instead try compiling the files (which is what we do in the build process currently) if the resulting JS output is not too ugly.

[bestwebdeveloper]

I tried to fix it as well but got stuck trying to understand the intention behind this and a way how to make sure that it still work properly.

Does anybody have understanding how it should work so we can implement it properly with proper super calls?
Or is there any test-cases/checklists how to check that everything is working fine after each change?

I believe there are not much cases which need to be fixed and they might be implemented this way not-intendedly, just occasionally.

[willroberts]

The testing process is generally this:

• Make sure yarn build works
• Make sure docker compose up works
• After the above two, go to http://localhost:3000 and play a practice game

There's more info on this in the Contributor Guide here:
https://github.com/open-duelyst/duelyst/blob/main/docs/CONTRIBUTING.md#setting-up-a-development-environment-
https://github.com/open-duelyst/duelyst/blob/main/docs/CONTRIBUTING.md#starting-the-game-locally-

I haven't looked closely at the super/this code yet, so I'm not yet sure what exactly is needed. But the above steps have been enough to know that it's working (or not).

[willroberts]

Upgrading CoffeeScript appears to result in significantly more memory usage. See #173 for details.

Getting rid of CoffeeScript via compiling it (instead of upgrading it to use decaffeinate) may be preferable here.

[KenAKAFrosty]

Would slowly decaffing the files work? Next time someone tries to edit a file, they could decaf it, add their logic, test and then push. JS files should still work with the other coffee files due to the interop.

I really like this approach, and I'll add that incorporating TypeScript to the migration early on would be preferable. If it's an incremental migration then the earlier the better.