Skip to content
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

Merged
merged 2 commits into from
May 16, 2022

Conversation

wagenet
Copy link
Member

@wagenet wagenet commented May 12, 2022

No description provided.

@wagenet wagenet changed the title Convert ember-testing to TS Convert ember-testing and @ember/test to TS May 12, 2022
Comment on lines +223 to +231
static initializer = buildInitializerMethod<'initializers', Application>(
'initializers',
'initializer'
);

static instanceInitializer = buildInitializerMethod<'instanceInitializers', ApplicationInstance>(
'instanceInitializers',
'instance initializer'
);
Copy link
Contributor

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'
  );

playground

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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?

Comment on lines +470 to +471
/** @internal */
export function buildInitializerMethod<
Copy link
Contributor

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.

@chriskrycho chriskrycho merged commit 27a42cd into emberjs:master May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants