Skip to content

Conversation

@sandersn
Copy link
Contributor

  1. Ember-data tests had the wrong return type in a couple of places. TS 3.4 catches this error, and this PR fixes it.
  2. Ember-data's Model class became invariant in 3.4 because of its use of the this type. This PR uses a this parameter with a method-local type parameter to avoid using Model's this type.
  3. Ember v2's tests were broken by TS 3.4's more complete return-type inference, which caused a variable declaration's type annotation to break inference through contextual typing. This PR converts the type annotation to type parameters on the call, which short-circuits inference entirely. It's also shorter.

After microsoft/TypeScript#29981 is merged, Ember types should compile cleanly with typescript@next.

1. Ember-data tests had the wrong return type in a couple of places. TS
3.4 catches this error, and this PR fixes it.
2. Ember-data's Model class became invariant in 3.4 because of its use of the
`this` type.  This PR uses a `this` parameter with a method-local type
parameter to avoid using Model's `this` type.
3. Ember v2's tests were broken by TS 3.4's more complete return-type
inference, which caused a variable declaration's type annotation to
break inference through contextual typing. This PR converts the type
annotation to type parameters on the call, which short-circuits
inference entirely. It's also shorter.
@sandersn
Copy link
Contributor Author

@mike-north This may not pass CI until tomorrow when Microsoft/TypeScript#29981 is in typescript@next, but maybe you can take a look now to see whether the changes make sense.

@typescript-bot
Copy link
Contributor

typescript-bot commented Feb 26, 2019

@sandersn Thank you for submitting this PR!

🔔 @jedmao @bttf @dwickern @chriskrycho @theroncross @mfeckie @alexlafroscia @mike-north @BryanCrotaz - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot
Copy link
Contributor

typescript-bot commented Feb 26, 2019

@sandersn The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Feb 28, 2019
@sandersn sandersn merged commit d2a5dcc into master Feb 28, 2019
@sandersn sandersn deleted the ember-ts-3.4-workarounds branch February 28, 2019 15:30
@typescript-bot
Copy link
Contributor

I just published @types/ember-data@3.1.4 to npm.

@typescript-bot
Copy link
Contributor

I just published @types/ember-data@2.14.21 to npm.

@mike-north
Copy link
Contributor

Thanks @sandersn this is a huge help!

@chriskrycho
Copy link
Contributor

Just want to reiterate @mike-north's comment: thank you so much for figuring this out and getting everything straightened out, @sandersn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Other Approved This PR was reviewed and signed-off by a community member.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants