Future @ember/component types#19948
Conversation
012a950 to
8e3a20b
Compare
ed8f5e7 to
bdb6580
Compare
bdb6580 to
4b1ccc2
Compare
chriskrycho
left a comment
There was a problem hiding this comment.
👍🏼 overall, couple small tweaks and then let's ship it.
|
|
||
| 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 😬
452237c to
aacca3b
Compare
chriskrycho
left a comment
There was a problem hiding this comment.
This should be the last bucket of fixes for this round—thank you for driving this forward.
|
|
||
| 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?
| 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.