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] Make types more strict in ra-core, part III #9760

Merged
merged 15 commits into from
Apr 9, 2024

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Apr 8, 2024

This PR finally turns strictNullChecks to true in ra-core, and solves the remaining 63 TS compilation errors on ra-core.

Refs #9622
Follows #9644, #9741 , #9743

useRecordContext Returns undefined When No Record Is Available

The useRecordContext hook reads the current record from the RecordContext. This context may be empty (e.g. while the record is being fetched). The return type for useRecordContext has been modified to Record | undefined instead of Record to denote this possibility.

As a consequence, the TypeScript compilation of your project may fail if you don't check the existence of the record before reading it.

To fix this error, your code should handle the case where useRecordContext returns undefined:

const MyComponent = () => {
    const record = useRecordContext();
+   if (!record) return null;
    return (
        <div>
            <h1>{record.title}</h1>
            <p>{record.body}</p>
        </div>
    );
};

</Table>
</div>
</DatagridRoot>
<OptionalResourceContextProvider value={resource}>
Copy link
Member Author

Choose a reason for hiding this comment

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

Stricter types revealed a bug: Using a Datagrid in controlled mode with an EditButton as a child used to fail silently (the EditButton link didn't work), because the resource set on the Datagrid was never propagated to the child. Now the failure is loud ans forces to add this ResourceContext.

@djhi djhi self-requested a review April 9, 2024 08:03
updateGroupState();
});
return unsubscribe;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we have an unsubscribe method. We should add one

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we do:

return () => {
subscribers.current[group] = subscribers.current[
group
].filter(s => s !== subscriber);
};

@@ -70,7 +70,7 @@ describe('<ReferenceArrayField />', () => {
record={{ id: 123, barIds: [1, 2] }}
reference="bar"
>
<SingleFieldList>
<SingleFieldList linkType={false}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary? linkType is optional in SingleFieldList props

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the test doesn't define a ResourceContext, and <SingleFieldList> defaults to creating a link. It's not possible to create a link without a Resource. Before this PR, this caused an empty/invalid link. Now, it throws an error. To avoid this error, we must disable the ink or define a ResourceContext.

@@ -157,6 +157,11 @@ export const useListParams = ({
tempParams.current = queryReducer(query, action);
// schedule side effects for next tick
setTimeout(() => {
if (!tempParams.current) {
throw new Error(
'Race condition in changeParams detected'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Users might need more information to solve this

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but this error should never occur (check the source code). To be honest, I added it just to please TypeScript.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that you mention it, throwing an error may break the app in legitimate cases (e.g. two successive changeParams calls). I've converted it to an early return.

@djhi djhi merged commit 3e810af into next Apr 9, 2024
12 checks passed
@djhi djhi deleted the fix-ts-strict-null-check-ra-core branch April 9, 2024 10:06
@djhi djhi added this to the 5.0.0 milestone Apr 12, 2024
Comment on lines +194 to +198
authProvider
? authProvider instanceof Function
? convertLegacyAuthProvider(authProvider)
: authProvider
: defaultAuthProvider,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just found out this is a BC, because now all admins have a default non-null authProvider.
So this breaks code like the following:

        <Root
            label={
                authProvider != null
                    ? 'ra.auth.user_menu'
                    : 'ra.configurable.customize'
            }
            icon={
                authProvider ? (
                    <Avatar src={identity.avatar} />
                ) : (
                    <SettingsIcon />
                )
            }
            {...props}
        />

Is there a way to avoid it?

Copy link
Contributor

Choose a reason for hiding this comment

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

#9861 attempts to fix this BC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants