Future @ember/component types #19948
Conversation
wagenet
force-pushed
the
ember-component-types
branch
3 times, most recently
from
012a950
to
8e3a20b
Compare
wagenet
force-pushed
the
ember-component-types
branch
2 times, most recently
from
ed8f5e7
to
bdb6580
Compare
wagenet
force-pushed
the
ember-component-types
branch
from
bdb6580
to
4b1ccc2
Compare
|
|
||
| attributeBindings?: Array<string>; | ||
| } | ||
| class Component |
There was a problem hiding this comment.
There is a (small) runtime risk here, if I remember correctly: there are some hazards around zebra-striping with classic classes. 🦓 CC @wycats @pzuraq @rwjblue @chancancode – I don’t remember what the issue was, but I do remember that we recommended against “zebra-striping” within chains, in the “it should work, 99% of the time, if you dot all your i’s and cross all your t’s but it’s easy to just not zebra-stripe” bucket. Does anyone remember the details, and if so is it an ongoing concern such that we shouldn’t do this conversion?
There was a problem hiding this comment.
To elaborate: the problem isn't internal, but for external consumers, since we currently still document Component.extend({ ... }) as the preferred way to define a Classic component.
There was a problem hiding this comment.
The issue is that some properties passed in extend won't override properties on the base class. I'll investigate these cases.
| renderer: Renderer; | ||
| } | ||
|
|
||
| export { CoreView as default }; |
There was a problem hiding this comment.
Why not export default CoreView;?
| // Glint would help with this | ||
| declare args: { number: number }; |
There was a problem hiding this comment.
This is for EmberComponent, right? Maybe just make some nonsense here that isn't args to avoid future confusion?
There was a problem hiding this comment.
Ah yeah, that's right. I always get them mixed up 😬
wagenet
force-pushed
the
ember-component-types
branch
from
452237c
to
aacca3b
Compare
| @@ -1,3 +1,6 @@ | |||
| import { Mixin } from '@ember/-internals/metal'; | |||
|
|
|||
| export default class ActionHandler extends Mixin {} | |||
| export default class ActionHandler extends Mixin { | |||
| actions?: Record<string, (...args: unknown[]) => void>; | |||
There was a problem hiding this comment.
-
Does it have to return
void(i.e. will anything else be thrown away anyway)? I think the answer is no, it can return anything. The type-checker will more or less shrug at it I think, but it should probably beunknown. -
I don't think the arguments will type check in practice either. 🤔 playground For types like this,
unknown[]is usually wrong for arguments, because it means subclasses can never have narrower types in the function parameters than than, because it breaks assignability/Liskov substitution/variance rules (whichever way is easiest to think about it for you; they all boil down to the same issue here).
Net, the type should probably be:
| actions?: Record<string, (...args: unknown[]) => void>; | |
| actions?: Record<string, (...args: any[]) => unknown>; |
(Technically, ...args: never[] is the "right" answer here, because all functions are valid super types of that, since never is the "bottom" type… but that confuses the heck out of people. And reasonably so.)
| export type EmberClassConstructor<T> = new (owner: Owner) => T; | ||
|
|
||
| type MergeArray<Arr extends any[]> = Arr extends [infer T, ...infer Rest] | ||
| export type MergeArray<Arr extends any[]> = Arr extends [infer T, ...infer Rest] |
There was a problem hiding this comment.
Should these also get @internal?
| @@ -42,7 +47,7 @@ const CoreView = FrameworkObject.extend(Evented, ActionHandler, { | |||
| */ | |||
| parentView: null, | |||
|
|
|||
| instrumentDetails(hash) { | |||
| instrumentDetails(hash: any) { | |||
There was a problem hiding this comment.
Fine to do a follow-on pass here, but would Record<string, unknown> work as a first pass?
Future @ember/component types
Future @ember/component types
No description provided.