[JSS] [React] - fix: type defs of React Placeholder render props

[Bram-Zijp]

The type definitions of the React Placeholder component were not user-friendly/stable. I ensured only one type was used and returned (React.ReactElement).

Motivation

Consistent types. No hacky type casting is done/required.

How Has This Been Tested?

Remove type casts and get TypeScript to test for you :+1:

Types of changes

• [x] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Documentation update (non-breaking change; modified files are limited to the /docs directory)

Checklist:

• [x] I have read the Contributing guide.
• [?] My code follows the code style of this project. -> This should be done automatically. I didn't see any code guidelines but free improvement remains free improvement. Feel free to adjust this mr to your liking.
• [ ] My code/comments/docs fully adhere to the Code of Conduct.
• [ ] My change is a code change and it requires an update to the documentation.
• [ ] My change is a documentation change and it requires an update to the navigation.

[anastasiya29]

This PR did not compile in CI

This is the error:

src/components/Placeholder.tsx(68,37): error TS2345: Argument of type '(ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<any, any, any>)> | null)[]' is not assignable to parameter of type 'ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<any, any, any>)>[]'.

Type 'ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<any, any, any>)> | null' is not assignable to type 'ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<any, any, any>)>'.
2021-02-22T16:37:49.2564342Z     Type 'null' is not assignable to type 'ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<any, any, any>)>'.

src/components/Placeholder.tsx(70,32): error TS2345: Argument of type '(ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<any, any, any>)> | null)[]' is not assignable to parameter of type 'ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)> | null) | (new (props: any) => Component<any, any, any>)>[]'.

Looking at the changes, it seems you may have been trying to fix some issues that have been covered here: #459 [sitecore-jss-react] Change propType of missingComponentComponent from React.SFC (deprecated) to React.FC. #538 [sitecore-jss-react] Change propType of errorComponent from React.SFC (deprecated) to React.FC.

Please let us know if you identified another issue

[Bram-Zijp]

I am not referencing any issues. This MR is not interested in SFC nor FC. The changes to the React propTypes are only done to improve the code base and are not the purpose of this MR. The main problem I had is the illustrated below;

Difference between rendering an element and a component:

const Component = () => <div />
const element = <Component />

// how to render the above:

const Example = () => 
  <>
     <Component />
     {element}
  </>

The type def React.ReactNode can be either an element or an Component. As you can see above, there's a huge difference in using an element over an Component.

As consumer you would have to add a check like this when the type defs say it can be either an element or an component:

React.isValidElement(elePassedByRenderEachHere) && <Spacing>{elePassedByRenderEachHere}</Spacing>

where as if it would be an element always per type definitions and implementation:

<Spacing>{elePassedByRenderEachHere}</Spacing>

One weird edge case that is not possible (returning strings in a component)

I will take a look at the type errors origin. This is probably a problem wide spread over your code base. I only noticed the Placeholder's function renderEach being typed on a smelly way. Considering Sitecore is commercial, it might be worth letting a Sitecore employed TypeScript dev take a look at these changes and the comments. Perhaps it needs to be fixed on a larger scale.

[Bram-Zijp]

I was just looking at this MR again and I am unable to reproduce these error's locally. Please take over this MR or let me know how to reproduce locally. Perhaps it's good to add these lint steps to the docs.