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

Make records paths inferred from type compatible with react-hook-form #10279

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

djhi
Copy link
Collaborator

@djhi djhi commented Oct 15, 2024

Problem

Fixes #9849

Records paths inferred from type using hotscript aren't compatible with those inferred by react-hook-form

Solution

Declare our own types for path inference that currently just re-export those from react-hook-form, making us more resilient to future changes.

How To Test

  • Open the InvoiceShow of the CRM demo and add a type parameter to the TextField of the Order reference. The TextField source should be valid and trying to replace it should trigger the IDE autocompletion, showing all Order fields

Additional Checks

  • The PR targets master for a bugfix, or next for a feature

packages/ra-ui-materialui/src/field/types.ts Show resolved Hide resolved
@@ -13,9 +14,11 @@ export interface RaRecord<IdentifierType extends Identifier = Identifier>
id: IdentifierType;
}

export type SortOrder = 'ASC' | 'DESC';
Copy link
Member

Choose a reason for hiding this comment

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

This type is already defined in ra-ui-materialui (packages/ra-ui-materialui/src/field/types.ts), I'm afraid of a conflic. As this change seems to be unrelated to the subject of the PR, could you revert it?

// Useful for props such as "source" in react-admin components
export type ExtractRecordPaths<T extends RecordValues> =
// Trick that allows to check whether T was provided
[T] extends [never] ? string : RecordPath<T>;
Copy link
Member

Choose a reason for hiding this comment

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

we used AnyString as the default value, now it's string. Isn't it a BC?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, AnyString is no longer necessary

Copy link
Collaborator Author

@djhi djhi Oct 16, 2024

Choose a reason for hiding this comment

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

More precisely, AnyString wasn't needed before. It was unnecessary already. It's only useful for HintedString

@fzaninotto
Copy link
Member

typecheck fails

@fzaninotto fzaninotto merged commit 480a5e0 into next Oct 16, 2024
14 checks passed
@fzaninotto fzaninotto deleted the remove-hotscript branch October 16, 2024 14:45
@fzaninotto fzaninotto modified the milestones: 5.2.4, 5.3.0 Oct 16, 2024
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.

2 participants