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

Preparation for typescript 2.4 #730

Merged
merged 2 commits into from Jun 19, 2017

Conversation

kwoon
Copy link
Contributor

@kwoon kwoon commented Jun 13, 2017

Typescript Changes

Please read about Week Types.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling bb496c8 on kwoon:preparation-for-typescript-2.4 into d213b08 on developit:master.

@jamesbirtles
Copy link
Contributor

I feel like this further supports #620

@marvinhagemeister
Copy link
Member

This PR fixes an issue we just stumbled upon at work:

48  abstract class Component<PropsType, StateType> implements ComponentLifecycle<PropsType, StateType> {
                   ~~~~~~~~~

node_modules/preact/dist/preact.d.ts(48,17): error TS2559: Type 'Component<PropsType, StateType>' has no properties in common with type 'ComponentLifecycle<PropsType, StateType>

+1 on merging this PR

@marvinhagemeister
Copy link
Member

@developit Any chance this might get merged? This makes preact work again with the latest TypeScript version.

@disintegrator
Copy link

disintegrator commented Jun 19, 2017

Great stuff! I also wanted a fix for the TS 2.4 compile error. While this does overcome the weak type overlap compile error, it does result in an unnecessary abstract class. ComponentLifecycle<P,S> isn't really an appropriate abstract class to expose to consumers as part of the public API. Furthermore, it is not referred to or extended by any type other than Component<P,S>. Perhaps, the more appropriate solution is to place all the lifecycle methods under Component<P,S> and then deleting ComponentLifecycle<P,S>. What do y'all think?

@marvinhagemeister
Copy link
Member

That sounds great as well! The ComponentLifeCycle interface only has one implementation anyway.

Copy link

@AdamMaras AdamMaras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new TypeScript language features are used in this PR. LGTM 👍

@developit developit merged commit b03bb36 into preactjs:master Jun 19, 2017
@valotas
Copy link
Contributor

valotas commented Jun 22, 2017

Not directly related, but shouldn't the props be of type PropsType & ComponentProps<this> for the ComponentLifecycle methods?

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.

None yet

8 participants