flarum / flarum/issue-archive

Various typing issues

Open
#96 4 comments 0 reactions 1 assignee Claimed by @davwheat View on GitHub
org/keep type/cleanup
Dominant language
No language data
Stars
0
Forks
0
PR merge metrics
No merged PRs in 30d

Description

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:

- [x] flarum/framework#2963
- [x] 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` or `MyUserPage extends UserPage`.
- [x] `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`.
- [x] `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`.
- [x] `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:

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

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

static hasMany(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

```ts
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:

```ts
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 https://github.com/flarum/core/issues/2343#issuecomment-826394029_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.