Typescript for models

[askvortsov1]

Supercedes #2411
Part of flarum/framework#3310
Part of flarum/issue-archive#96

Changes proposed in this pull request:
Sorry in advance, this is a long one.

Convert Model.ts, Store.ts, and all models to TypeScript.

Reviewers should focus on:
As can be seen in the "Typecheck" GH action, this PR is sound from a type integrity perspective. There are, however a few concerns I have.

First and foremost, please take a look at the util types/interfaces I've added to Store and Model, their naming, and whether everything that needs to be exported is actually being exported.

Secondly, is there anything missing in the Store util types that should be there in order to comply with the JSON:API spec?

Thirdly, the types for computed are very crude. Is there a better way to do this in a typesafe way?

Fourthly, there currently isn't anything that enforces integrity of whether a model is saved being consistent with exists. One suggestion was to add a Saved extends boolean generic param to Model, and use conditional types to govern the types of exists and data. This makes things really messy though, as every subclass also needs to specify this, and this as a return type doesn't support swapping types when needed.

Fithly, dealing with singular vs plural types with Store.pushPayload is a pain. See Application.preloadedApiDocument and Store.find for an example. If anyone has ideas for a cleaner way to do this (or a way to constrain issues to Store.pushPayload) that would be fantastic.

Finally, and this is the biggest one, I've typed all model attributes as safely as I can: | null is added to all nullable fields, and | undefined is added to all fields not included on the model's BaseSerializer. This is a bit clunky and overly broad, but it's safe. One idea I had to get around this is the concept of SerializerProfiles: this would be a generic argument to models representing which fields are present on a given serializer, and their types. It would then be used to generate all typings on all attributes. This sounds great in concept, but when trying to implement it, I had to use a lot of very ugly mapped and conditional types, as well as some very verbose generics and naming. As a result, I don't think it's worth going down that route.

Necessity

• Has the problem that is being solved here been clearly explained?
• If applicable, have various options for solving this problem been considered?
• For core PRs, does this need to be in core, or could it be in an extension?
• Are we willing to maintain this for years / potentially forever?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).
• Core developer confirmed locally this works as intended.
• Tests have been added, or are not appropriate here.

Required changes:

• Related documentation PR: (Remove if irrelevant)
• Related core extension PRs: (Remove if irrelevant)

[askvortsov1]

I did a lot of refactoring on this method to make it more understandable, less hacky, and more typesafe. This one needs a bit of extra review.

[SychO9]

Only reviewed about 65% of this so far :P

[SychO9]

wouldn't it be more friendly for attributes/relationships to write instead:

Suggested change

	title() {
	return Model.attribute<string>('title').call(this);
	}
	title = Model.attribute<string>('title');

[dsevillamartin]

@SychO9 This breaks monkey patching I believe. Class properties are not in .prototype and thus cannot be extended easily (if at all (?))

From Clark's PR

The difference between Object.assign/mixin and the proposed solution is that the attributes are no longer part of the model prototype and as such cannot be easily overridden by extensions. However I don't think any extension ever attempted that and I'm not sure why the "mixin" style was used at all in the past.

More @ #2411 (comment)

[SychO9]

ah shame :(, the easier the code is the easier it is for new extension devs to get into it. But being able to monkey patch these is important as well.

[askvortsov1]

Personally, I don't think model attributes should really be monkey patched (that's a VERY unsafe approach to development), just that for now we need to maintain BC.

[dsevillamartin]

It will create a difference between ext and core attributes, as we extend Models with prototype, and once we switch over to class properties, those would be separate from the prototype. Don't think this would cause issues, but it might be confusing.

And yeah, think I'm for changing it in a breaking release as it's very ugly and repetitive.

[askvortsov1]

I'm hoping that by the time 2.0 rolls around, we will have proper JS extenders for this.