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

[TypeScript] Fix FunctionField render function type expects a nullable record #8963

Merged

Conversation

elstgav
Copy link
Contributor

@elstgav elstgav commented May 31, 2023

When using Typescript, the FunctionField render prop currently requires redundant null checks, as it claims record can be undefined. However it’s guaranteed not to, thanks to the null check on line 30:

record ? (
<Typography
component="span"
variant="body2"
className={className}
{...sanitizeFieldRestProps(rest)}
>
{render(record, source)}
</Typography>
) : null,

This same guarantee applies to <WithRecord>, which is documented here:

Note that if `record` is undefined, `<WithRecord>` doesn't call the `render` callback and renders nothing, so you don't have to worry about this case in your render callback.

This PR updates the render prop type to clarify that record will be defined, eliminating the need for redundant null checks:

  <FunctionField
    source='user'
-   render={(record) => record && record.first_name}
+   render={(record) => record.first_name}
  />

The PR also:

  • creates a RenderResourcesFunction type, that can be used when creating custom render methods
  • Clarifies that the render method must return a valid ReactNode, not any.
  • Updates example docs to remove redundant null checks (this was previously inconsistent across FunctionField examples)

@@ -133,9 +133,7 @@ const Contact = ({ note }: any) => (
>
<FunctionField<ContactType>
variant="body2"
render={(contact?: ContactType) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ContactType is already set by line 134

export type FunctionFieldRenderer<
RecordType extends Record<string, unknown> = Record<string, any>
> = (record: RecordType, source?: string) => React.ReactNode;
Copy link
Contributor Author

@elstgav elstgav May 31, 2023

Choose a reason for hiding this comment

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

I was unsure on what to name this 🤔. It may be helpful to further abstract this into a type that’s shared by <WithRecord>

@fzaninotto
Copy link
Member

Great PR, thanks!

I think we can safely merge it to master, as it's more a fix of the existing type. Besides, the current type has just changed (see #8964), so you'll need to adjust your code a bit.

Would you mind rebasing the PR on master?

Comment on lines -17 to +25
export const WithRecord = <RecordType extends RaRecord>({
export const WithRecord = <RecordType extends Record<string, unknown> = any>({
render,
}: WithRecordProps<RecordType>) => {
const record = useRecordContext<RecordType>();
return record ? render(record) : null;
};

export interface WithRecordProps<RecordType extends RaRecord> {
render: (record: RecordType) => ReactElement;
export interface WithRecordProps<
RecordType extends Record<string, unknown> = any
> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated these to match #8964

Copy link
Contributor Author

@elstgav elstgav Jun 1, 2023

Choose a reason for hiding this comment

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

@fzaninotto @slax57 it might be helpful to create a “strict” RaRecord type that we can share/extend from? Could name it StrictRaRecord, or TypedRaRecord, etc

e.g.

// ra-core/src/types.ts

export interface StrictRaRecord<IdentifierType extends Identifier = Identifier> {
    id: IdentifierType;
}

export interface RaRecord<IdentifierType extends Identifier = Identifier> extends StrictRaRecord {
    [key: string]: any;
}

// in other files…

export interface WithRecordProps<RecordType extends StrictRaRecord = RaRecord> {  }
export interface FunctionFieldProps<RecordType extends StrictRaRecord = RaRecord> {  }

// When typing field components…

interface Post extends StrictRaRecord<number> {
    title: string;
    teaser: string;
    body: string;
    published_at: string;
}

export const PostShow = () => (
    <Show>
        <SimpleShowLayout>
            <TextField<Post> source="title" />
            <TextField<Post> source="teaser" />
            {/* Here TS will show an error because a teasr field does not exist */}
            <TextField<Post> source="teasr" />
            <RichTextField<Post> source="body" />
            <DateField<Post> label="Publication date" source="published_at" />
        </SimpleShowLayout>
    </Show>
);

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure of the added value (extending a StrictRaRecord vs adding an Identifier by hand is about the same). Besides, it's a more general reflexion about struct null checks that we're working on in the long term.

Comment on lines +364 to +367
export type RenderRecordFunction<
RecordType extends Record<string, unknown> = any
> = (record: RecordType, source?: string) => ReactNode;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if there’s a more appropriate place to put this. Named to match the existing RenderResourcesFunction:

export type RenderResourcesFunction = (

@elstgav
Copy link
Contributor Author

elstgav commented Jun 1, 2023

@fzaninotto rebased, and created a shared RenderRecordFunction type that is shared with the WithRecord component.

@fzaninotto fzaninotto merged commit c238fd5 into marmelab:master Jun 2, 2023
@fzaninotto
Copy link
Member

Thanks!

@fzaninotto fzaninotto added this to the 4.11.2 milestone Jun 2, 2023
@fzaninotto fzaninotto changed the title Remove redundant null checks from FunctionField render type [TypeScript] Fix FunctionField render function type expects a nullable record Jun 2, 2023
@elstgav elstgav deleted the function-field-render-prop-types branch June 2, 2023 13:35
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.

2 participants