Various typing issues

[davwheat]

Copied from @clarkwinkelmann's comment on flarum/framework#2343.

---

Here's a list of things I noticed related to the types package. Some of them are noticeable just by using the types package, other only start causing issues when manually enabling typescript on the extension.

I have not checked if any has been already fixed in dev-master. Those issues were present in beta 16:

• Globals variables ($, dayjs) are not correctly type-hinted framework#2963
• The way the global app object is defined in core shims.d.ts uses common/Application for both forum/app and admin/app, which means that all special objects are not usable, like app.history and app.extensionData. I worked around it by type-hinting it as app: ForumApplication & AdminApplication but it's not ideal. There should be a different type-hint for each side. This will probably be fixed by using the forum/app or admin/app every time instead of the global.
• Most of the Flarum components that aren't currently written in typescript don't have a view() method in their signature, which cause typescript to complain that "abstract method view() is not implemented" when extending them. Including Modal and UserPage.
• Some Flarum components don't carry over the generic type for ComponentAttrs, so it's impossible to customize it. That's probably because those components aren't themselves written with typescript. For example it's impossible to have MyModal extends Modal<MyModalAttrs> or MyUserPage extends UserPage<MyUserPageAttrs>.
• Modal.onsubmit() is incorrectly type-hinted without any parameter, so it fails when we use the event parameter. It should be type-hinted as event: Event.
• Translator.trans() is incorrectly type-hinted with second parameter required. Should be trans(id: any, parameters?: any)
• username() is incorrectly type-hinted as User being non-nullable, but it's often used for missing users and renders the [deleted] text for those. Type should probably be something like username(user: User | null | undefined | false) because it can be null when directly using an API value, or undefined/false when retrieved from the store.
• app.current.data is type-hinted as {} instead of any, so trying to access any property on it results in errors like Property 'routeName' does not exist on type '{}'.
• app.session.user is type-hinted as any | null instead of expected User | null.
• static Component.component has type-hint attrs: {} = {} instead of expected attrs: T = {} (assuming we can type-hint static methods with generics?).
• None of the attribute-pseudo-props are type-hinted on the models. Like User.prototype.username(), and all methods on Discussion, Forum, etc.
• Model 's type-hint on static methods is wrong, see below.
• Model.prototype.delete() has required parameter body which should be optional
• static Component.component() has children required as a null type. This is a new issue with dist-typings that wasn't present with flarum/types
• The first argument to static Component.component() should use typing T to enforce the attr interface on usage

The case of Model static methods

The method incorrectly type-hint the output of the method when it should type-hint the output of the anonymous methods returned by those methods.

This causes a call like myModel.user() to throw an error "Cannot invoke an object which is possibly undefined" because it thinks myModel.user might be undefined and not a method, when it's actually the result of () which might be undefined.

I also think those methods should support a generic so extensions can define the return type of the anonymous function. This is what I used as a workaround for now:

export default class Model extends OriginalModel {
    static attribute<T = any>(name: string, transform?: Function): (value?: string) => T | undefined {
        return OriginalModel.attribute(name, transform);
    }

    static hasOne<T extends OriginalModel = OriginalModel>(name: string): () => T | false | undefined {
        return OriginalModel.hasOne(name) as any;
    }

    static hasMany<T extends OriginalModel = OriginalModel>(name: string): () => T[] | false {
        return OriginalModel.hasMany(name);
    }
}

Loaded/oninit attributes

One problem I have faced in multiple components are temporarily nullable attribute.

I often have variables that are only initialized in oninit, but if I just type-hint them as their final values in the class, typescript complains I didn't allow for the undefined value they will have between the constructor and oninit. I have found !: to be a solution to these, like

class MyComponent extends Component {
    tag!: Tag;

    oninit (vnode) {
        super.oninit(vnode);

        this.tag = this.attrs.tag;
    }
}

However I have not yet found a good solution for attribute that are null until the first render, like this:

class MyComponent extends Component {
    tag: Tag | null = null;

    oninit (vnode) {
        super.oninit(vnode);

        // Simulate network request
        setTimeout(() => {
            this.tag = this.attrs.tag;
            m.redraw();
        }, 1000);
    }

    view () {
        if (!this.tag) {
            return LoadingIndicator.component();
        }

        return Button.component({
            onclick: () => {
                // Typescript will complain that this.tag could be null, because this runs inside of a callback
                // and the value might be null again. However we know the value will never get back to null at this point.
                alert(this.tag.name());
            },
        }, this.tag.name()); // This is fine, because typescript sees the check for null above in the same function
    }
}

I'm not yet sure how to write this without creating a second component that takes the variable as an attribute from the same component and make it non-nullable.

Originally posted by @clarkwinkelmann in flarum/framework#2343 (comment)

[clarkwinkelmann]

Edited by davwheat: these have been added to the OP task list

A few more:

• Model.prototype.delete() has required parameter body which should be optional
• static Component.component() has children required as a null type. This is a new issue with dist-typings that wasn't present with flarum/types
• The first argument to static Component.component() should use typing T to enforce the attr interface on usage

Another thing that requires consideration is how to type-hint Store.pushPayload(). I manually typed the output as MyModel[] but that doesn't cover the .payload property that gets added to the array. This method will probably need a <T> property that then translates to an interface that's the T[] & the payload: any property.

[clarkwinkelmann]

Other thing: we need a type-hint for app under common namespace. In 1.0+ there's no way to import app there. flarum/app has no typings, and importing flarum/admin/app causes error in forum and vice-versa.

[davwheat]

In regards to the app typings, I think we should remove them from the global typings (which should be fixed to actually work in extensions soon), and instead enforce importing from flarum/xxx/app.

If we don't we'd need to do something like:

declare const app: import('../src/common/app') | import('../src/forum/app') | import('../src/admin/app')

...which doesn't seem to be liked by TS (causes about 400+ typing errors).

[clarkwinkelmann]

I used & between the app imports when I made by local types declarations for this. Of course it has the downside of allowing use of methods that are not actually available in the current frontend.

As long as there's an app in all namespaces including common, I think it's fine if we drop them, at least from the typings.

The global app object is useful for tests using the browser console, but in the code itself it shouldn't have a big impact to actually import it, since that'll be compressed anyway.