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

[pickers] Improve PickerValidDate type #14771

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Sep 30, 2024

Fixes #14765

If no adapter has registered a date type in PickerValidDate, then we fallback to any instead of never.

@flaviendelangle flaviendelangle changed the title [pickers] Improve PickerValidDate [pickers] Improve PickerValidDate Sep 30, 2024
@flaviendelangle flaviendelangle self-assigned this Sep 30, 2024
@flaviendelangle flaviendelangle added typescript component: pickers This is the name of the generic UI component, not the React module! labels Sep 30, 2024
@mui-bot
Copy link

mui-bot commented Sep 30, 2024

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Nice improvement! 🙌 💯

@LukasTy LukasTy changed the title [pickers] Improve PickerValidDate [pickers] Improve PickerValidDate type Sep 30, 2024
@flaviendelangle
Copy link
Member Author

@LukasTy how would you handle the doc section?
For people with the new version of the package, it make no sense since the error no longer exists.
But for people with "older" version they can still find it in the doc and it can help them.

Maybe we can let it as is and during the alpha we re-word it to remove the "error" aspect and just present how to correctly type if people have a "any" as the date type.

@LukasTy
Copy link
Member

LukasTy commented Sep 30, 2024

Maybe we can let it as is and during the alpha we re-word it to remove the "error" aspect and just present how to correctly type if people have a "any" as the date type.

What do you think about updating the second paragraph of the doc section to reflect these new changes in this PR? 🤔
Something like:

If you see `value` or other similar props typed as `any` it means that there is no `LocalizationProvider` with a defined adapter in your TypeScript project scope.
You can fix it by manually importing the adapter in some file of your project as follows:

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Sep 30, 2024

If you see value or other similar props typed as any

People won't directly see, they will see the type they pass to their prop.

To take a very dumb example:

image

But they can import PickerValidDate to check:

image

So maybe:

If you are not sure your adapter is set up correctly to infer the type of the value prop (or other similar props),
you can import the PickerValidDate type and check its current value:

If its equal to any, then you can try to import manually your adapter as show below:
image

If it's equal to the date object used by our library, then you don't have to do anything:
image

@LukasTy
Copy link
Member

LukasTy commented Sep 30, 2024

People won't directly see, they will see the type they pass to their prop.

Oh, that's not the best DX if it's not that clear. 🙈

Well, if we can't improve this behavior, then indeed, your suggestion with code examples makes sense. 👍

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Sep 30, 2024

The other solution I see is to remove the TDate generic and just use PickerValidDate everywhere.
In OUR codebase it would mean that we could no longer limit the valid value to be Dayjs or Date for example, because we have all the adapters loaded.
But in a real application it would simplify the typing and make your proposal possible.

This is typing breaking change, but one I'm totally willing to make 👍

@LukasTy
Copy link
Member

LukasTy commented Oct 1, 2024

But in a real application it would simplify the typing and make your proposal possible.

This is typing breaking change, but one I'm totally willing to make 👍

That sounds great. Can we do this for v8? 🤔
Shouldn't have too much of an impact for most users anyways. 👍

@flaviendelangle
Copy link
Member Author

Can we do this for v8? 🤔

Sure
I finish #14651 and we review it before the alpha
That way I can work on this PR during the alpha without massive Git conflicts on every single interface 😆

@flaviendelangle
Copy link
Member Author

@LukasTy are you good merging this PR as is or do you think we should keep it for v8 and the typing changes ?

@LukasTy
Copy link
Member

LukasTy commented Oct 1, 2024

are you good merging this PR as is or do you think we should keep it for v8 and the typing changes ?

WDYT about the documentation situation?
Maybe we should go with your suggestions for the v7 and update if we change the usage to directly use the PickerValidDate? 🤔

@flaviendelangle
Copy link
Member Author

👍 Right, I'll update the doc

@flaviendelangle
Copy link
Member Author

I tried to add some explanation on the doc 👍

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Superb improvement on doc! 👍 💯

docs/data/date-pickers/base-concepts/base-concepts.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] The value prop is not correctly typed
3 participants