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

Use React.FC for PanelStack2 renderPanel typing to fix compilation on newer @types/react versions #6521

Merged

Conversation

gluxon
Copy link
Contributor

@gluxon gluxon commented Nov 7, 2023

Context

A semi-recent change in @types/react switched React.FunctionComponent's return type from ReactElement to ReactNode.

 interface FunctionComponent<P = {}> {
-    (props: P, context?: any): ReactElement<any, any> | null;
+    (props: P, context?: any): ReactNode;
     propTypes?: WeakValidationMap<P> | undefined;
     contextTypes?: ValidationMap<any> | undefined;
     defaultProps?: Partial<P> | undefined;
     displayName?: string | undefined;
 }

Break

Under @types/react@16.14.32 and @types/react@18.0.25, the following minimal example from the docs passes TypeScript compilation.

import { Panel, PanelProps } from "@blueprintjs/core";
import {} from "@blueprintjs/core";

type AccountSettingsPanelInfo = {
  /* ...  */
};

const AccountSettingsPanel: React.FC<PanelProps<AccountSettingsPanelInfo>> = (
  props
) => {
  return null;
};

const _panel: Panel<unknown> = {
  renderPanel: AccountSettingsPanel,
  title: "Account settings"
};

However, on @types/react@18.2.34, it fails with:

Type 'FC<PanelActions>' is not assignable to type '(props: PanelActions) => Element | null'.
  Type 'ReactNode' is not assignable to type 'Element | null'.
    Type 'undefined' is not assignable to type 'Element | null'.ts(2322)

Changes

This PR fixes compilation of panel stack components under newer versions of @types/react by following React.FC's typings.

Notes for Reviews

My initial attempt to fix this was to add React.ReactNode as another possible renderPanel return type. While this works on newer versions of @types/react, it breaks on older versions.

Screenshot 2023-11-06 at 7 26 51 PM

@adidahiya
Copy link
Contributor

Follow `React.FC` return type for `renderPanel` typings

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor

Hmm, that's strange, I can see the type error in the sandbox repro you linked.

But when I tried to repro it myself, I don't get that error: https://codesandbox.io/p/sandbox/react-18-panelstack2-types-debugging-pcdslz?file=%2Fsrc%2FCoreExample.tsx%3A64%2C11

There are a bunch of places in the codebase where we expect a return type of JSX.Element, does this mean we have to update all of those too?

@gluxon
Copy link
Contributor Author

gluxon commented Nov 7, 2023

Hmm, that's strange, I can see the type error in the sandbox repro you linked.

But when I tried to repro it myself, I don't get that error: https://codesandbox.io/p/sandbox/react-18-panelstack2-types-debugging-pcdslz?file=%2Fsrc%2FCoreExample.tsx%3A64%2C11

That's really surprising. Taking a look, I see there's 2 definitions of FunctionComponent in @types/react.

Counterintuitively, I think the ts5.0 dir is for TypeScript versions before 5. From https://unpkg.com/@types/react@18.2.36/package.json

{
    "exports": {
        ".": {
            "types@<=5.0": {
                "default": "./ts5.0/index.d.ts"
            },
            "types": {
                "default": "./index.d.ts"
            }
        }
        // ...
    }
    // ...
}

The playground in the link above is using the ts5.0/index.d.ts types, which still have function components returning React.ReactElement.

Screenshot 2023-11-07 at 11 52 11 AM

I checked locally and see the issue on TypeScript 5.1.6.

There are a bunch of places in the codebase where we expect a return type of JSX.Element, does this mean we have to update all of those too?

I think the renderPanel error showed more frequently in the codebase I was looking at because the docs had the React.FC annotation on panel components, and developers in the past followed the Blueprint docs. I don't expect to see this problem for other renderers like renderDialog, which will likely be implemented as arrow functions that have the return type inferred.

renderDialog?: (state: HotkeysContextState, contextActions: { handleDialogClose: () => void }) => JSX.Element;

I think we still have a bit more time to decide the best next step; I wouldn't want us to rush replacing JSX.Element and regret it later.

@@ -21,7 +21,7 @@ export interface Panel<P> {
/**
* The renderer for this panel.
*/
renderPanel: (props: PanelProps<P>) => JSX.Element | null;
renderPanel: (props: PanelProps<P>) => ReturnType<React.FC<PanelProps<P>>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this could also just use React.FC entirely.

Suggested change
renderPanel: (props: PanelProps<P>) => ReturnType<React.FC<PanelProps<P>>>;
renderPanel: React.FC<PanelProps<P>>;

But that is a bit more strict than what we had before this PR since React.FC has additional optional properties on it.

interface FunctionComponent<P = {}> {
    (props: P, context?: any): ReactNode;
    propTypes?: WeakValidationMap<P> | undefined;
    contextTypes?: ValidationMap<any> | undefined;
    defaultProps?: Partial<P> | undefined;
    displayName?: string | undefined;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this out and it seems to work fine with both functions that are annotated as React.FC and plain (un-annotated) ones which return any of the ReactNode types: https://codesandbox.io/p/sandbox/react-18-panelstack2-types-debugging-typescript-v5-x-hz29zl?file=%2Fsrc%2FCoreExample.tsx%3A18%2C1

so I think we can go ahead with this suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. Thank you for the help testing that React.FC<...> works for those cases. PR should be updated for this now.

@gluxon gluxon force-pushed the bcheng/update-render-panel-typings-for-new-fc-typings branch from b9f25ad to e0b8098 Compare November 8, 2023 21:23
@gluxon gluxon marked this pull request as ready for review November 8, 2023 21:25
@gluxon gluxon changed the title Follow React.FC return type for renderPanel typings to fix compilation on newer @types/react versions Use React.FC for PanelStack2 renderPanel typing to fix compilation on newer @types/react versions Nov 8, 2023
@adidahiya
Copy link
Contributor

Use `React.FC` for PanelStack2 `renderPanel` prop typing

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya adidahiya merged commit 7d12971 into develop Nov 9, 2023
@adidahiya adidahiya deleted the bcheng/update-render-panel-typings-for-new-fc-typings branch November 9, 2023 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants