CODEBASE: Fix lint errors 3

[catloversg]

This is a part of the series of PRs for #1730.

Providing type check for usages of libarg is too messy, so I gave up and disabled lint rules for those cases.

In NetscriptDefinitions.d.ts, we have some declare enum looked like this:

declare enum UniversityLocationName {
  AevumSummitUniversity = LocationName.AevumSummitUniversity,
  Sector12RothmanUniversity = LocationName.Sector12RothmanUniversity,
  VolhavenZBInstituteOfTechnology = LocationName.VolhavenZBInstituteOfTechnology,
}

These enums will trigger the @typescript-eslint/prefer-literal-enum-member rule. IMO, this kind of usage is fine, so I disable that rule for NetscriptDefinitions.d.ts.

Typing for IReviverValue is not very good. I'm not satisfied with the way I did, but I have not found a better way. The type of data can be:

• Normal object (Record<string, any>): This is good for most classes that will be serialized to JSON in the save data.
• Array<any>: JSONSet.
• Array<[any, any]>: JSONMap.

I have not found a way to define T in IReviverValue, so I "lie" to TS and set it as Record<string, any>, and then typecasting in JSONSet and JSONMap. I'm open to other suggestions.
Edit: I tried to improve the type checking in 1f8ed02.

[d0sboots]

I haven't finished the whole thing, but I'm gonna stop and send comments now because there's really two parts to this PR:

1. A bunch of minor lint fixes that I have no issues with.
2. A type refactoring for the save/load stuff. My issue here (as mentioned in some comments) is that our loading is not typesafe in actuality; anything can be in the savegame (and semi-frequently is, when people do savegame editing ineptly). As a result, representing all of these JSON'd structures as specific types is simply a lie, and the lie is occurring at all the points where you do casts. I'm actually more comfortable with the old state of the code, where we were doing a bad thing but at least we weren't lying about it. The new lint errors are (rightly) warning us about the bad things we're doing here, and I'd rather they continue to do that until we fix our issues. (Or, we disable them line-by-line and admit that we won't.)

[catloversg]

Before I start making changes, I want to make sure that we agree on how to deal with these problems:

IReviverValue

If we leave <T = any> as it is right now, I'll disable @typescript-eslint/no-unsafe-argument line-by-line in all toJSON and fromJSON.
If we change it to <T = unknown>, I'll have to use // @ts-expect-error -- reason line-by-line in all toJSON and fromJSON.
You have to make a decision here. In the case that you choose <T = unknown>, if you don't mind, please write the "reason" for me. I'll copy it.

evaluateVersionCompatibility in SaveObject.ts

I agree with your comment, but we still have to find a viable way to deal with the lint errors here. If we don't lie to TS, there will be tons of lint errors. Disabling all of them line-by-line is tedious and "weird". Another option is to move this function to a new file, then disable rules in that file. This option is not ideal, but it's still better than dumping tons of // eslint-disable-next-line to this function.

[d0sboots]

There is a third option, but it might be too much work (it would belong in its own PR for sure): Fixing the lint errors by fixing the code, i.e. properly checking types on load.

[catloversg]

Gradually dealing with fromJSON and toJSON is possible, but what about evaluateVersionCompatibility in SaveObject.ts? Checking all possibly legacy properties of anyPlayer sounds like a hellish job. I'm not even sure how to start with it.

[d0sboots]

Well, the idea would be to check at time-of-use. I.e., you don't assert it has a particular shape up-front, but rather let it be unknown and check for the presence of fields as you go. This is how the NetscriptHelper functions work, for instance. And the js code already mostly works this way, it's just making some assumptions about "if it has this field, it must be this object that also has these fields."

At the same time, I'm not too concerned about the legacy conversion code; it gets run rarely and is very hard to test, so changing it minimally is usually best. I'm fine with wholesale disabling lint on it one way or another. It's the contemporary loading code that I'd potentially like to tighten up.

[catloversg]

New changes:

• Revert e7de968 and some other changes in SaveObject.ts. I'll make those changes in another PR.
• Remove usages of throwErrorIfNotObject in most fromJSON. Some classes still call that function because they need to assert value.data before using it (besides Generic_fromJSON).
• Change objectAssert, arrayAssert, stringAssert: throw an error instead of a string. I did it in a way that it would not affect current callers of those functions.

About fromJSON in Jsonable.ts: value.data is unknown, so TS won't let us pass it JSONMap (same thing for JSONSet). We have 2 choices:

• Typecasting without checking. Error handling will rely on Map/Set constructor. [1]
• Typecasting after our own checking. [2]

I'm not particularly against [1], but I'm hesitant about doing it. This is an example of how things may go wrong (it's about JSONSet, not JSONMap):

• Create a corporation.
• Edit the save file: Search: "JSONSet" of "researched". Change data from [] to \"test\".
• Load the edited save file.
• Check the console: ResearchTree.research() did not find the specified Research node for:

The game loads normally, but the research data is corrupted.

I know that the example is weird. If the player improperly edits their save file, that's their problem. My point is that the Set constructor does not check the same thing like we do (it will take a string and use it as an array of characters). Maybe there are problems with the Map constructor too.

Nonetheless, I have not found a problem with the Map constructor, so I can do [1] if you insist, but I still prefer [2].

[d0sboots]

OK, that's a sufficiently compelling example. Technically it is perfectly legal to create a set from a string, but is rarely what is wanted, and always incorrect in our usage.

[d0sboots]

I have one comment, but f it. Let's goooooo

[gmcew]

I'll happily admit I am well out of my depth here; but isn't this still needed by the first import line of NetscriptFunctions.ts? The tests don't get upset, but trying to run the game without it throws an error for me.

[d0sboots]

I was under the assumption that CatLover had tested thoroughly, since he is usually quite thorough about these things. Could it be a cached artifact issue (does it still happen from a clean tree)?

[d0sboots]

On the latest head, after npm install npm run start:dev seems to work fine for me.

[catloversg]

@gmcew It may be a problem with your node_modules folder. Can you delete it, then run npm i + npm run build again?