Skip to content

typings: add context as 3rd type parameter for the Component interface - #1725

Open
mk0x9 wants to merge 1 commit into
preactjs:mainfrom
mk0x9:typings-for-context
Open

typings: add context as 3rd type parameter for the Component interface#1725
mk0x9 wants to merge 1 commit into
preactjs:mainfrom
mk0x9:typings-for-context

Conversation

@mk0x9

@mk0x9 mk0x9 commented Jun 20, 2019

Copy link
Copy Markdown

So TypeScript compiler will type check it :)

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 99.674% when pulling a2d701e on mk0x9:typings-for-context into f39f877 on preactjs:master.

Comment thread src/index.d.ts
componentWillReceiveProps?(nextProps: Readonly<P>, nextContext: any): void;
shouldComponentUpdate?(nextProps: Readonly<P>, nextState: Readonly<S>, nextContext: any): boolean;
componentWillUpdate?(nextProps: Readonly<P>, nextState: Readonly<S>, nextContext: any): void;
componentWillReceiveProps?(nextProps: Readonly<P>, nextContext: C): void;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thinking we should mark context as Readonly here like props and state are treated.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Makes sense, thanks!

Comment thread src/index.d.ts
state: Readonly<S>;
props: RenderableProps<P>;
context: any;
context: C;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This might be odd when using contextType? Not sure. Using contextType assigns this.context to the current value of the given context provider.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You mean that it's possible to assign any Context to the static contextType? Or that contextType could be dynamically switched?

@pmkroeker

Copy link
Copy Markdown
Contributor

It would be great if this could be looked at again. Currently using contextType does not set the type of context in a class. It does set the proper type of class.contextType.

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.

5 participants