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

Add TS support to fields #8863

Merged
merged 35 commits into from
May 23, 2023
Merged

Add TS support to fields #8863

merged 35 commits into from
May 23, 2023

Conversation

djhi
Copy link
Collaborator

@djhi djhi commented Apr 27, 2023

Follow #8862

If users provide types extending RaRecord, nothing changes, they won't have TS validation of the source prop but they won't have any error either. Same when they don't provide types.

If users provide types that don't extend RaRecord, they'll get validation and completion on the source prop

};
ArrayFieldImpl.propTypes = fieldPropTypes;
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 to clone the prop types before. There was probably a reason, I think you shouldn't change that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't think of a valid reason. We do the same in EmailField.

I think someone did not simplify the syntax when removing an old propType specific to the ArrayField that did not make sense in v4. See f7d9f18

Copy link
Member

Choose a reason for hiding this comment

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

I vaguely remember having to clone proptypes for an obscure reason. Let's revert it if we have no reason to do it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What about the EmailField, UrlField and WrapperField then?

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep them as in master. I know, in master there is no consistency. Let's keep it like that.

My opinion is that this PR changes quite a lot of stuff, and we should avoid all unnecessary changes to reduce any potential BC break.

packages/ra-ui-materialui/src/field/ArrayField.tsx Outdated Show resolved Hide resolved
examples/crm/src/contacts/TagsListEdit.tsx Outdated Show resolved Hide resolved
packages/ra-core/src/types.ts Outdated Show resolved Hide resolved
packages/ra-ui-materialui/package.json Outdated Show resolved Hide resolved
@fzaninotto fzaninotto merged commit ff2133b into next May 23, 2023
@fzaninotto fzaninotto deleted the ts-record-reference-fields branch May 23, 2023 09:36
@fzaninotto fzaninotto added this to the 4.11.0 milestone May 23, 2023
@akillingbeck89
Copy link

Just upgraded from 4.5.1 to 4.11.1 and having TS issues all over the place where I set my "source" parameter, is there a way to disable this?

@slax57
Copy link
Contributor

slax57 commented Jun 9, 2023

@akillingbeck89 This change should be backwards compatible. If it is not, this is to be considered as a bug.
Can you please open a new issue with more information about the errors you are getting, and ideally a reproduction sandbox?
This way we can better grasp what's going on and offer adequate help.
For instance, it would help to know if you are passing custom types to the fields or not.

@akillingbeck89
Copy link

akillingbeck89 commented Jun 9, 2023

Dont have time right now, will do it next week, but no custom types are being passed and an example error I'm getting is:

Type '"channelId"' is not assignable to type '[${key}] | [${key}].${string} | null'.ts(2322)

@grevolution
Copy link

Dont have time right now, will do it next week, but no custom types are being passed and an example error I'm getting is:

Type '"channelId"' is not assignable to type '[${key}] | [${key}].${string} | null'.ts(2322)

How did you fix this issue?

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

Successfully merging this pull request may close these issues.

5 participants