Open davwheat opened 3 years ago
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 optionalstatic Component.component()
has children
required as a null
type. This is a new issue with dist-typings
that wasn't present with flarum/types
static Component.component()
should use typing T
to enforce the attr interface on usageAnother 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.
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.
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).
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.
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:
app
object is defined in coreshims.d.ts
usescommon/Application
for bothforum/app
andadmin/app
, which means that all special objects are not usable, likeapp.history
andapp.extensionData
. I worked around it by type-hinting it asapp: ForumApplication & AdminApplication
but it's not ideal. There should be a different type-hint for each side. This will probably be fixed by using theforum/app
oradmin/app
every time instead of the global.view()
method in their signature, which cause typescript to complain that "abstract method view() is not implemented" when extending them. IncludingModal
andUserPage
.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 haveMyModal extends Modal<MyModalAttrs>
orMyUserPage extends UserPage<MyUserPageAttrs>
.Modal.onsubmit()
is incorrectly type-hinted without any parameter, so it fails when we use theevent
parameter. It should be type-hinted asevent: Event
.Translator.trans()
is incorrectly type-hinted with second parameter required. Should betrans(id: any, parameters?: any)
username()
is incorrectly type-hinted asUser
being non-nullable, but it's often used for missing users and renders the[deleted]
text for those. Type should probably be something likeusername(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 ofany
, so trying to access any property on it results in errors likeProperty 'routeName' does not exist on type '{}'
.app.session.user
is type-hinted asany | null
instead of expectedUser | null
.static Component.component
has type-hintattrs: {} = {}
instead of expectedattrs: T = {}
(assuming we can type-hint static methods with generics?).User.prototype.username()
, and all methods onDiscussion
,Forum
, etc.Model
's type-hint on static methods is wrong, see below.Model.prototype.delete()
has required parameterbody
which should be optionalstatic Component.component()
haschildren
required as anull
type. This is a new issue withdist-typings
that wasn't present withflarum/types
static Component.component()
should use typingT
to enforce the attr interface on usageThe case of
Model
static methodsThe 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 thinksmyModel.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:
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 theundefined
value they will have between the constructor andoninit
. I have found!:
to be a solution to these, likeHowever I have not yet found a good solution for attribute that are
null
until the first render, like this: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