Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
8108d36
Add definitions for React.memo
Jessidhia Oct 24, 2018
3519670
Add missing semicolons
Jessidhia Oct 24, 2018
9ea5462
Give a better name to the second argument to React.memo
Jessidhia Oct 24, 2018
5fdb7dc
Fix test to reflect correct usage of React.memo's second argument
Jessidhia Oct 24, 2018
66909c0
Fix no-unnecessary-qualifier lints
Jessidhia Oct 24, 2018
5a081ad
Update other special components to be SpecialSFC
Jessidhia Oct 24, 2018
a6b4dc7
Ensure ordinary functions aren't assignable to SpecialSFC
Jessidhia Oct 24, 2018
d066793
createElement was resolving P to "{} | null" in some tests; add exten…
Jessidhia Oct 24, 2018
fa0a57d
Fix the props of Fragment and StrictMode
Jessidhia Oct 24, 2018
c47014e
Add tests for SpecialSFC assignability
Jessidhia Oct 24, 2018
055cc6d
Add scary doc comment to SpecialSFC's call signature
Jessidhia Oct 24, 2018
25c2862
Rename SpecialSFC to ExoticComponent
Jessidhia Oct 24, 2018
88991b0
Add overload to React.memo to make it more ergonomic and avoid implic…
Jessidhia Oct 30, 2018
67a704f
Disable test that requires TS 3.1
Jessidhia Oct 30, 2018
4c49d3a
Add support for displayName in the exotic components that support it
Jessidhia Nov 2, 2018
0a10ba0
Tone down the call signature's doc comment
Jessidhia Nov 9, 2018
bab3ed0
Correct $ExpectType assertion
Jessidhia Nov 9, 2018
299c1f2
Try to implement LibraryManagedAttributes for the ExoticComponents th…
Jessidhia Oct 26, 2018
2b9cb56
Add type definitions for new React 16.6 features
Jessidhia Oct 26, 2018
02fdb38
The fallback prop is required by Suspense
Jessidhia Oct 30, 2018
3a26c43
Declare legacy context on tests that use legacy context
Jessidhia Nov 9, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion types/react-virtualized/react-virtualized-tests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ export class ArrowKeyStepperExample extends PureComponent<any, any> {
import { List } from "react-virtualized";

export class AutoSizerExample extends PureComponent<any, any> {
context: any;
state: any;
render() {
const { list } = this.context;
Expand Down Expand Up @@ -200,6 +201,7 @@ const GUTTER_SIZE = 3;
const CELL_WIDTH = 75;

export class CollectionExample extends PureComponent<any, any> {
context: any;
state: any;
_columnYMap: any;

Expand Down Expand Up @@ -372,6 +374,7 @@ export class ColumnSizerExample extends PureComponent<any, any> {
}

export class GridExample extends PureComponent<any, any> {
context: any;
state = {
columnCount: 1000,
height: 300,
Expand Down Expand Up @@ -511,7 +514,7 @@ const STATUS_LOADING = 1;
const STATUS_LOADED = 2;

export class InfiniteLoaderExample extends PureComponent<any, any> {

context: any;
state: any;
_timeoutIds = new Set<number>();

Expand Down Expand Up @@ -619,6 +622,7 @@ export class InfiniteLoaderExample extends PureComponent<any, any> {
}

export class ListExample extends PureComponent<any, any> {
context: any;
state: any;
constructor(props: any, context: any) {
super(props, context);
Expand Down Expand Up @@ -758,6 +762,7 @@ export class GridExample2 extends PureComponent<any, any> {
_cellPositioner: Positioner;
_masonry: Masonry;

context: any;
state: any;

constructor(props: any, context: any) {
Expand Down Expand Up @@ -1256,6 +1261,7 @@ function mixColors(color1: any, color2: any, amount: any) {
import { Column, Table, SortDirection, SortIndicator } from "react-virtualized";

export class TableExample extends PureComponent<{}, any> {
context: any;
state = {
disableHeader: false,
headerHeight: 30,
Expand Down Expand Up @@ -1498,6 +1504,7 @@ export class DynamicHeightTableColumnExample extends PureComponent<any, any> {

export class WindowScrollerExample extends PureComponent<{}, any> {
_windowScroller: WindowScroller;
context: any;
state = {
showHeaderText: true
};
Expand Down
162 changes: 141 additions & 21 deletions types/react/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,19 +191,19 @@ declare namespace React {
...children: ReactNode[]): DOMElement<P, T>;

// Custom components
function createElement<P>(
function createElement<P extends {}>(
type: SFC<P>,
props?: Attributes & P | null,
...children: ReactNode[]): SFCElement<P>;
function createElement<P>(
function createElement<P extends {}>(
type: ClassType<P, ClassicComponent<P, ComponentState>, ClassicComponentClass<P>>,
props?: ClassAttributes<ClassicComponent<P, ComponentState>> & P | null,
...children: ReactNode[]): CElement<P, ClassicComponent<P, ComponentState>>;
function createElement<P, T extends Component<P, ComponentState>, C extends ComponentClass<P>>(
function createElement<P extends {}, T extends Component<P, ComponentState>, C extends ComponentClass<P>>(
type: ClassType<P, T, C>,
props?: ClassAttributes<T> & P | null,
...children: ReactNode[]): CElement<P, T>;
function createElement<P>(
function createElement<P extends {}>(
type: SFC<P> | ComponentClass<P> | string,
props?: Attributes & P | null,
...children: ReactNode[]): ReactElement<P>;
Expand Down Expand Up @@ -255,11 +255,40 @@ declare namespace React {
unstable_observedBits?: number;
}

type Provider<T> = ComponentType<ProviderProps<T>>;
type Consumer<T> = ComponentType<ConsumerProps<T>>;
// TODO: similar to how Fragment is actually a symbol, the values returned from createContext,
// forwardRef and memo are actually objects that are treated specially by the renderer; see:
// https://github.com/facebook/react/blob/v16.6.0/packages/react/src/ReactContext.js#L35-L48
// https://github.com/facebook/react/blob/v16.6.0/packages/react/src/forwardRef.js#L42-L45
// https://github.com/facebook/react/blob/v16.6.0/packages/react/src/memo.js#L27-L31
// However, we have no way of telling the JSX parser that it's a JSX element type or its props other than
// by pretending to be a normal component.
//
// We don't just use ComponentType or SFC types because you are not supposed to attach statics to this
// object, but rather to the original function.
interface ExoticComponent<P = {}> {
/**
* **NOTE**: Exotic components are not callable.
*/
(props: P): (ReactElement<any>|null);
readonly $$typeof: symbol;
}

interface NamedExoticComponent<P = {}> extends ExoticComponent<P> {
displayName?: string;
}

interface ProviderExoticComponent<P> extends ExoticComponent<P> {
propTypes?: ValidationMap<P>;
}

// NOTE: only the Context object itself can get a displayName
// https://github.com/facebook/react-devtools/blob/e0b854e4c/backend/attachRendererFiber.js#L310-L325
type Provider<T> = ProviderExoticComponent<ProviderProps<T>>;
type Consumer<T> = ExoticComponent<ConsumerProps<T>>;
interface Context<T> {
Provider: Provider<T>;
Consumer: Consumer<T>;
displayName?: string;
}
function createContext<T>(
defaultValue: T,
Expand All @@ -269,8 +298,25 @@ declare namespace React {
function isValidElement<P>(object: {} | null | undefined): object is ReactElement<P>;

const Children: ReactChildren;
const Fragment: ComponentType;
const StrictMode: ComponentType;
const Fragment: ExoticComponent<{ children?: ReactNode }>;
const StrictMode: ExoticComponent<{ children?: ReactNode }>;
/**
* This feature is not yet available for server-side rendering.
* Suspense support will be added in a later release.
*/
const Suspense: ExoticComponent<{
children?: ReactNode

/** A fallback react tree to show when a Suspense child (like React.lazy) suspends */
fallback: NonNullable<ReactNode>|null
Copy link
Copy Markdown
Member Author

@Jessidhia Jessidhia Oct 30, 2018

Choose a reason for hiding this comment

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

React will throw at runtime if the Suspense suspends and the fallback is undefined, but null is ok.


// I tried looking at the code but I have no idea what it does.
// https://github.com/facebook/react/issues/13206#issuecomment-432489986
/**
* Not implemented yet, requires unstable_ConcurrentMode
*/
// maxDuration?: number
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't know whether to leave this in or not. The code to support it technically exists, but is unreachable without unstable_ConcurrentMode which we are not defining here.

}>;
const version: string;

//
Expand All @@ -283,6 +329,29 @@ declare namespace React {
// tslint:disable-next-line:no-empty-interface
interface Component<P = {}, S = {}, SS = any> extends ComponentLifecycle<P, S, SS> { }
class Component<P, S> {
// tslint won't let me format the sample code in a way that vscode likes it :(
/**
* If set, `this.context` will be set at runtime to the current value of the given Context.
*
* Usage:
*
* ```ts
* type MyContext = number
* const Ctx = React.createContext<MyContext>(0)
*
* class Foo extends React.Component {
* static contextType = Ctx
* context!: MyContext
* render () {
* return <>My context's value: {this.context}</>;
* }
* }
* ```
*
* @see https://reactjs.org/docs/context.html#classcontexttype
*/
static contextType?: Context<any>;

constructor(props: Readonly<P>);
/**
* @deprecated
Expand All @@ -308,11 +377,6 @@ declare namespace React {
// on the existence of `children` in `P`, then we should remove this.
readonly props: Readonly<{ children?: ReactNode }> & Readonly<P>;
state: Readonly<S>;
/**
* @deprecated
* https://reactjs.org/docs/legacy-context.html
*/
context: any;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I removed this to make sure there's no magic any in this.context. That'll only happen when using legacy context; if using modern context it'll be the type of the value prop of this.constructor.contextType.Provider.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, it is strange, cause here https://reactjs.org/docs/context.html#classcontexttype we can see, React allows us to use just this.context, but it is not possible, if context will be deleted from types of Component.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By the way, there is no anything like value in Component.contextType.Provider.

Copy link
Copy Markdown
Member Author

@Jessidhia Jessidhia Oct 30, 2018

Choose a reason for hiding this comment

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

The point is to force the user to declare it themselves because there is nothing better we can do other than any here, and having it already declared would allow people to access it even without a static contextType.

value is the value prop of the Provider exotic component. The actual type of context would be something like typeof this.constructor.contextType extends React.Context<infer T> ? T : never if this.constructor would be useful (its typeof is just Function).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, could you explain, why this code is not working:

class Component<P, S> {
    static contextType: Context<any>;
    ...
    ...
    context: typeof Component.contextType extends Context<infer T> ? T : never
}

Type of context is any always( But it works perfectly in my own component.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is because, by declaring it in the superclass, you don't have access to its type as assigned in the subclass. The type of the constructor itself is not generic so it just uses the type as declared, assuming Component is constructed as is and not through a subclass. (Note: despite how I phrased it, this still wouldn't work if Component was abstract)

This is the deficiency in TypeScript's class declaration language: it is possible to make an instance type generic, but not the constructor object itself generic.

There is a possible hack with constructible interfaces, though, if they're even accepted in an extends clause. It could be possible to just completely replace the class Component declaration with a pair of interfaces and a const. But... at what cost?.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for explanation. I agree, what it is ok to get rid of context from typings.

/**
* @deprecated
* https://reactjs.org/docs/refs-and-the-dom.html#legacy-api-string-refs
Expand Down Expand Up @@ -417,6 +481,7 @@ declare namespace React {
// Unfortunately, we have no way of declaring that the component constructor must implement this
interface StaticLifecycle<P, S> {
getDerivedStateFromProps?: GetDerivedStateFromProps<P, S>;
getDerivedStateFromError?: GetDerivedStateFromError<P, S>;
}

type GetDerivedStateFromProps<P, S> =
Expand All @@ -427,6 +492,15 @@ declare namespace React {
*/
(nextProps: Readonly<P>, prevState: S) => Partial<S> | null;

type GetDerivedStateFromError<P, S> =
/**
* This lifecycle is invoked after an error has been thrown by a descendant component.
* It receives the error that was thrown as a parameter and should return a value to update state.
*
* Note: its presence prevents any of the deprecated lifecycle methods from being invoked
*/
(error: any) => Partial<S> | null;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I actually don't know if this is allowed to return null 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the argument be of type Error?

Suggested change
(error: any) => Partial<S> | null;
(error: Error) => Partial<S> | null;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I kept it the same type as catch has.

try {
  ...
} catch (e) {
  // e will _always_ be `any`
  // giving it another type is a build error
}

You can also throw anything in javascript; even throw undefined.


// This should be "infer SS" but can't use it yet
interface NewLifecycle<P, S, SS> {
/**
Expand Down Expand Up @@ -558,7 +632,45 @@ declare namespace React {

function createRef<T>(): RefObject<T>;

function forwardRef<T, P = {}>(Component: RefForwardingComponent<T, P>): ComponentType<P & ClassAttributes<T>>;
// will show `ForwardRef(${Component.displayName || Component.name})` in devtools by default,
// but can be given its own specific name
interface ForwardRefExoticComponent<P> extends NamedExoticComponent<P> {
defaultProps?: Partial<P>;
}

function forwardRef<T, P = {}>(Component: RefForwardingComponent<T, P>): ForwardRefExoticComponent<P & ClassAttributes<T>>;

type ComponentProps<T extends ComponentType<any>> =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need <any> here (and in the few other instances below)?
It looks like ComponentType generic will default to {} anyway, so maybe you could omit it?
See here:

type ComponentType<P = {}> = ComponentClass<P> | StatelessComponent<P>;

Copy link
Copy Markdown
Member Author

@Jessidhia Jessidhia Nov 1, 2018

Choose a reason for hiding this comment

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

Yeah, if there is no argument it'll be inferred as {} and this will actually cause the compiler to reject subtypes of ComponentType<{}> because {} will be a function or constructor argument's type.

It's the problem of (arg: { foo: string }) => null not being assignable to (arg: {}) => null, but being assignable to (arg: any) => null.

T extends ComponentType<infer P> ? P : {};
type ComponentPropsWithRef<T extends ComponentType<any>> =
T extends ComponentClass<infer P>
? P & ClassAttributes<InstanceType<T>>
: T extends SFC<infer P>
? P
: {};

// will show `Memo(${Component.displayName || Component.name})` in devtools by default,
// but can be given its own specific name
interface MemoExoticComponent<T extends ComponentType<any>> extends NamedExoticComponent<ComponentPropsWithRef<T>> {
readonly type: T;
}

function memo<P extends object>(
Component: SFC<P>,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this right? If I have a render prop component (where children might be a function), it's not being inferred as functional component.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Implicit children strike back

Again

🙄

I guess I have a New Years break project for a breaking change. Hopefully one I can get to compile.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm investigating it further. The problem may not be children, but something else (my current bet is functional components with custom generic types). I'll see if I can come up with a minimum reproduction.

Copy link
Copy Markdown
Member Author

@Jessidhia Jessidhia Dec 25, 2018

Choose a reason for hiding this comment

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

@sandersn @johnnyreilly Would it be possible to condition this breaking change on a specific TypeScript version through typesVersion?

I was thinking of being able to make the change incremental by only updating types on demand, when people update to, say, TS3.3. This would require kind of a "max supported TS version" on types that depend on @types/react.

I'm a bit afraid of it being a python3 problem, though, but the technical debt of @types/react is now nigh-unmanageable.

Yes, I could work around this problem in this specific instance by changing the input to a function instead of FunctionComponent, but it's pervasive and everywhere. A lot of things that currently work in the ecosystem (like function children) work because of ossified bugs in the types, not because of features. @types/react is due for a rewrite.

Copy link
Copy Markdown
Member Author

@Jessidhia Jessidhia Dec 25, 2018

Choose a reason for hiding this comment

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

@diegohaz ah, yes, that is the one big problem with mapped types and infer (and the lack of kinds?); they erase generics and collapse unions. If the props are either of those the output type may be funky.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In another thread we decided it was time to update the types to expect TS 3

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.

Would it be possible to condition this breaking change on a specific TypeScript version through typesVersion?

I'm afraid I haven't played with typesVersion yet and so can't advise. That said, as @ericanderson mentioned there's been prior discussion about upgrading to TS 3. We should do that anyway. If anyone is thinking about submitting a PR that does that then they should expect ♥️ and 🤗

propsAreEqual?: (prevProps: Readonly<P & { children?: ReactNode }>, nextProps: Readonly<P & { children?: ReactNode }>) => boolean
): NamedExoticComponent<P>;
function memo<T extends ComponentType<any>>(
Component: T,
propsAreEqual?: (prevProps: Readonly<ComponentProps<T>>, nextProps: Readonly<ComponentProps<T>>) => boolean
): MemoExoticComponent<T>;

interface LazyExoticComponent<T extends ComponentType<any>> extends ExoticComponent<ComponentPropsWithRef<T>> {
readonly _result: T;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not really happy about this. Having an underscored value here looks like reaching into internals; but it's the only way to have access to T in LibraryManagedProps.

An alternative is to make up our own fictional prop name.

}

function lazy<T extends ComponentType<any>>(
factory: () => Promise<{ default: T }>
): LazyExoticComponent<T>;

//
// React Hooks
Expand Down Expand Up @@ -2460,6 +2572,14 @@ type Defaultize<P, D> = P extends any
& Partial<Pick<D, Exclude<keyof D, keyof P>>>
: never;

type ReactManagedAttributes<C, P> = C extends { propTypes: infer T; defaultProps: infer D; }
? Defaultize<MergePropTypes<P, PropTypes.InferProps<T>>, D>
: C extends { propTypes: infer T; }
? MergePropTypes<P, PropTypes.InferProps<T>>
: C extends { defaultProps: infer D; }
? Defaultize<P, D>
: P;

declare global {
namespace JSX {
// tslint:disable-next-line:no-empty-interface
Expand All @@ -2470,13 +2590,13 @@ declare global {
interface ElementAttributesProperty { props: {}; }
interface ElementChildrenAttribute { children: {}; }

type LibraryManagedAttributes<C, P> = C extends { propTypes: infer T; defaultProps: infer D; }
? Defaultize<MergePropTypes<P, PropTypes.InferProps<T>>, D>
: C extends { propTypes: infer T; }
? MergePropTypes<P, PropTypes.InferProps<T>>
: C extends { defaultProps: infer D; }
? Defaultize<P, D>
: P;
// We can't recurse forever because `type` can't be self-referential;
// let's assume it's reasonable to do a single React.lazy() around a single React.memo() / vice-versa
type LibraryManagedAttributes<C, P> = C extends React.MemoExoticComponent<infer T> | React.LazyExoticComponent<infer T>
? T extends React.MemoExoticComponent<infer U> | React.LazyExoticComponent<infer U>
? ReactManagedAttributes<U, P>
: ReactManagedAttributes<T, P>
: ReactManagedAttributes<C, P>;

// tslint:disable-next-line:no-empty-interface
interface IntrinsicAttributes extends React.Attributes { }
Expand Down
15 changes: 15 additions & 0 deletions types/react/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -715,3 +715,18 @@ class RenderChildren extends React.Component {
return children !== undefined ? children : null;
}
}

const Memoized1 = React.memo(function Foo(props: { foo: string }) { return null; });
React.createElement(Memoized1, { foo: 'string' });

const Memoized2 = React.memo(
function Bar(props: { bar: string }) { return null; },
(prevProps, nextProps) => prevProps.bar === nextProps.bar
);
React.createElement(Memoized2, { bar: 'string' });

const specialSfc1: React.ExoticComponent<any> = Memoized1;
const sfc: React.SFC<any> = Memoized2;
// this $ExpectError is failing on TypeScript@next
// // $ExpectError Property '$$typeof' is missing in type
// const specialSfc2: React.SpecialSFC = props => null;
Loading