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 1f8ed0231d24eb0482a4247efc9cb021c3aba748.

[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 e7de9688868bf8e1cc04a9b8f7bd499f90db4a56 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.