whscullin / apple2js

An Apple II emulator originally written in Javascript, now being converted to TypeScript
http://www.scullinsteel.com/apple2/
MIT License
442 stars 57 forks source link

Add the recommended eslint plugins for TypeScript #119

Closed iflan closed 2 years ago

iflan commented 2 years ago

This adds both the recommended TypeScript checks, plus the recommended TypeScript checks that require type checking. This latter addition means that eslint essentially has to compile all of the TypeScript in the project, causing it to be slower. This isn't much of a problem in VS Code because there's a lot of caching being done, but it's clearly slower when run on the commandline.

All of the errors are either fixed or suppressed. Some errors are suppressed because fixing them would be too laborious for the little value gained. Others are caused by some overly-zealous matching that requires a config refactoring to fix.

iflan commented 2 years ago

⚠️ I do not like some of the recommended settings:

For all of these settings, I would be happy to revert the changes and disable the rule. If we think that the rule is fine in general, but not for a subset of files, that's fine with me, too. (For example, the no-non-null-assertion rule could be turned off for the /ui/ package.) If we do that, I'll do a general refactor of the eslint settings to make it easier to change in the future.

whscullin commented 2 years ago

I have a bit of deja vu about maybe having tried to turn this on before and having mixed feelings. I tend to agree with you, I like explicit types, and while I kind of get the rational for "no empty interfaces" I do like laying out structure with them. Also, an empty interface is guarding against stray assignments in a way that Record does not. The no non-null assertions are also frustrating because the code written without them either becomes a sea of optional chaining operators or extremely verbose. However, as we transition away from direct DOM manipulation that becomes less of an issue. I think turning it off for the /ui/ directory would make sense.

iflan commented 2 years ago

This is obsolete with #121 merged.