Add typings for class component state attribute

[davwheat]

Changes proposed in this pull request:

All of our class component instances have a state attribute which is, by default, undefined. This is managed by Mithril itself, but we currently have no way to apply types to it within Flarum.

This PR adds the ability to set typings for this attribute more easily, so that extensions can choose to store the data for their components within this.state more easily.

We generally store component data directly on the this object, but this often ends up polluting the class with lots of data. Storing it under state makes it easier to be typed, especially if data is set dynamically, and it is more friendly to developers coming from other frameworks, such as React.

We might want to consider defaulting this.state to an empty object within our class component. This shouldn't cause any backwards-compatibility issues, but might be better in another PR.

Current method:

class MyComponent extends Component<Record<string, never>> {
  loading!: boolean;
  apiData?: Record<string, unknown>;
  formData!: {
    icon: string,
  };

  oninit() {
    super.oninit(arguments);
    this.loading = true;
    this.formData = {
      icon: "fas fa-blah",
    };
  }

  stopLoading() {
    this.loading = false;
  }

  async fetchApiData() {
    this.apiData = await m.request("https://example.com/endpoint");
  }
}

this.state method:

interface IComponentState {
  loading: boolean;
  apiData?: Record<string, unknown>;
  formData: {
    icon: string;
  }
}

class MyComponent extends Component<Record<string, never>, IComponentState> {
  oninit() {
    super.oninit(arguments);

    this.state = {
      loading: true,
      formData: {
         icon: "fas fa-blah"
      },
    };
  }

  stopLoading() {
    this.state.loading = false;
  }

  async fetchApiData() {
    this.apiData = await m.request("https://example.com/endpoint");
  }
}

Confirmed

• Frontend changes: tested on a local Flarum installation.

[askvortsov1]

Are there benefits to using this.state over this aside from typing? Types on class properties aren't that difficult IMO. I'm hesitant to introduce support for this approach unless we go 100% in and discourage setting properties directly on the component object.

[davwheat]

There isn't any inherent benefit, no.

The reason I added this was because it's part of mithril, like attrs and lifecycle hooks.

I would rather discourage setting values on the class instance itself and instead use state. IMO, it makes it clearer to understand.

[askvortsov1]

I would rather discourage setting values on the class instance itself and instead use state. IMO, it makes it clearer to understand.

I think I agree with this. However, I'm not sure we should make this change now until we're ready to start refactoring core. Maybe we should work on this after the TS refactor?

[davwheat]

I wouldn't want to work on it until we start proper development on 2.0. It'd be a breaking change for extensions that hook into existing component state if we moved it all to a state object.

[askvortsov1]

I wouldn't want to work on it until we start proper development on 2.0. It'd be a breaking change for extensions that hook into existing component state if we moved it all to a state object.

Maybe then we should include this in the 2.0 refactors?

[davwheat]

In my opinion, it'd be better to add it now. It's one extra thing out of the way, and something that extension developers and new core components can use to be ready for 2.0 and reduce the upgrade workload.