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

Users shouldn't need 'noImplicitThis' on to benefit from 'ThisType<T>' #14518

Closed
DanielRosenwasser opened this issue Mar 7, 2017 · 4 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

From #14141:

  • Otherwise, if --noImplicitThis is enabled and the containing object literal has a contextual type that includes a ThisType<T>, this has type T.

Is there a reason that users need noImplicitThis on to benefit from ThisType<T>? This seems unnecessarily restrictive, especially given that we want to give better support in JS files.

@DanielRosenwasser DanielRosenwasser added the Bug A bug in TypeScript label Mar 7, 2017
@ahejlsberg
Copy link
Member

@DanielRosenwasser There are multiple new inferences made for this in #14141 and, except for the one where we look for a ThisType<T> in the contextual type, they are breaking changes. I suppose we could consider always applying the ThisType<T> rule but not applying the other two rules unless --noImplicitThis is enabled. Is that what you are suggesting? It seems odd to separate it like that. Or are you suggesting that we should always strongly type this and live with the breakage?

@ahejlsberg ahejlsberg added Suggestion An idea for TypeScript and removed Bug A bug in TypeScript labels Mar 8, 2017
@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Mar 8, 2017

I suppose we could consider always applying the ThisType<T> rule but not applying the other two rules unless --noImplicitThis is enabled. Is that what you are suggesting?

Yup, sounds like what I had in mind. The thing is that we need to enable this logic for Salsa users anyway, and if a library author wrote ThisType<T> somewhere, they probably meant to use it.

I suppose there is a chance that library authors using the ThisType type could potentially break existing code that relies on this being typed as any, but I do wonder how often that comes up in practice. Presumably ThisType will be used in contexts where it makes sense to apply it.

@ahejlsberg
Copy link
Member

Indeed, even ThisType<T> might break existing code. So each of the new inferences in #14141 is potentially a breaking change. But treating ThisType<T> differently just seems really confusing. Users don't really care how the type of this got inferred, they just care whether it is strongly typed or not. I think we either stick with the current scheme (my preference) or we take the breaking change and always have strongly typed this.

@RyanCavanaugh RyanCavanaugh added the Declined The issue was declined as something which matches the TypeScript vision label Aug 13, 2018
@RyanCavanaugh
Copy link
Member

Doesn't really seem worth a breaking change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants