Convert engines, applications, and more to TS #2
Conversation
| if (this.isDestroyed) { | ||
| throw new Error(`Cannot call \`.factoryFor\` after the owner has been destroyed`); | ||
| } | ||
| let normalizedName = this.registry.normalize(fullName); | ||
|
|
||
| assert('fullName must be a proper full name', this.registry.isValidFullName(normalizedName)); | ||
|
|
||
| // TODO: This needs to return a Factory to be compatible with Owner. | ||
| // We should correctly the types so that this cast is not necessary. | ||
| return factoryFor(this, normalizedName, fullName) as Factory<unknown>; |
| @@ -11,36 +13,21 @@ export interface FactoryClass { | |||
| positionalParams?: string | string[] | undefined | null; | |||
| } | |||
|
|
|||
| export interface Factory<T, C extends FactoryClass | object = FactoryClass> { | |||
| export interface Factory<T extends object, C extends FactoryClass | object = FactoryClass> { | |||
There was a problem hiding this comment.
Currently, the code looks as if it only supports objects here. If emberjs#19532 we need to revisit this, but having it all in TS should actually make it easier!
| @@ -81,13 +79,12 @@ const VALID_FULL_NAME_REGEXP = /^[^:]+:[^:]+$/; | |||
| */ | |||
| export default class Registry implements IRegistry { | |||
| readonly _failSet: Set<string>; | |||
| resolver: Resolver | null; | |||
| resolver: Resolver | (Resolve & NotResolver) | null; | |||
There was a problem hiding this comment.
| @@ -560,25 +553,28 @@ if (DEBUG) { | |||
| }; | |||
| } | |||
|
|
|||
| function resolve<T, C>(registry: Registry, _normalizedName: string): Factory<T, C> | undefined { | |||
There was a problem hiding this comment.
Good riddance to unsafe generics.
|
|
||
| if (mountPoint) { | ||
| state.engineBucket = { mountPoint }; | ||
| } |
There was a problem hiding this comment.
I believe this was incorrect before. Looking at the code, it looks like engineBucket should never be set without mountPoint.
| @@ -35,10 +44,10 @@ export const ActionManager: { | |||
| export declare class EventDispatcher extends EmberObject { | |||
| events: Record<string, string>; | |||
| finalEventNameMapping: Record<string, string>; | |||
| rootElement: string | HTMLElement; | |||
| rootElement: string | Element; | |||
There was a problem hiding this comment.
Is this loosening correct?
| setupHandlerForEmberEvent(event: string): void; | ||
|
|
||
| /** @private */ | ||
| setup(addedEvents: Record<string, string | null>, rootElement: string | Element | null): void; |
There was a problem hiding this comment.
I don't think we want to declare this twice!
| @@ -52,7 +69,7 @@ const ApplicationInstance = EngineInstance.extend({ | |||
| @private | |||
| @property {Object} customEvents | |||
| */ | |||
| customEvents: null, | |||
| customEvents: Record<string, string | null> | null = null; | |||
There was a problem hiding this comment.
This needs to be a declare to allow overwriting.
| @@ -61,10 +78,10 @@ const ApplicationInstance = EngineInstance.extend({ | |||
| @private | |||
| @property {String|DOMElement} rootElement | |||
| */ | |||
| rootElement: null, | |||
| rootElement: string | Element | null = null; | |||
There was a problem hiding this comment.
This needs to be a declare to allow overwriting.
| }, | ||
|
|
||
| // We overwrote this method from RegistryProxyMixin. | ||
| this.__registry__.unregister(fullName); |
There was a problem hiding this comment.
This is a bit awkward but it's the best idea I have.
wagenet
force-pushed
the
application-engine-ts
branch
from
47f62a5
to
e266525
Compare
Requires emberjs#19964.