-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert ember-testing and @ember/test to TS #20090
Conversation
5ef4f0b
to
d4e944a
Compare
d4e944a
to
09a73ad
Compare
static initializer = buildInitializerMethod<'initializers', Application>( | ||
'initializers', | ||
'initializer' | ||
); | ||
|
||
static instanceInitializer = buildInitializerMethod<'instanceInitializers', ApplicationInstance>( | ||
'instanceInitializers', | ||
'instance initializer' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I poked at why this is the way it is, and I believe if we update how buildInitializerMethod
is defined, we can drop all the casting shenanigans. 😅
// Two significant changes here:
//
// 1. Only parameterize over the bucket name, and simply *infer* the type of
// initializer from it using the bucket name.
// 2. Make the return type explicit, so that the type parameterization can fall
// out from it naturally. that the actual returned function can be simpler,
// not having to do the type parameterization on it.
function buildInitializerMethod<
B extends 'initializers' | 'instanceInitializers'
>(
bucketName: B,
humanName: string
): (this: typeof Engine, initializer: Initializer<Kind<B>>) => void {
return function (initializer) {
// ...
};
}
type Kind<B extends 'initializers' | 'instanceInitializers'> =
B extends 'initializers' ? Engine : EngineInstance;
Then the usage here will be:
static initializer = buildInitializerMethod('initializers', 'initializer');
static instanceInitializer = buildInitializerMethod(
'instanceInitializers',
'instance initializer'
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems likely. I didn’t try very hard beyond getting it to work, but it does seem worth trying to clean it up a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this works except for one significant problem: I think we want initializers on Application to return Application/ApplicationInstance not Engine/EngineInstance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Hmm. 🤔 That’s… actually impossible to guarantee from a types POV given the existing API, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For posterity: We spent some time poking at this and it is indeed impossible to guarantee the correct behavior here… because, and only because, it requires inference on the this
type, and that interacts badly with Object.prototype.call
. 😩 Net, we'll leave it as is and… hope for a day when initializers are dead?
/** @internal */ | ||
export function buildInitializerMethod< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per comments above, we should update this to be smarter about how it does its thing.
No description provided.