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

[Community help wanted] useComponentDefaultProps types with strict mode in tsconfig? #3717

Closed
7iomka opened this issue Mar 8, 2023 · 10 comments

Comments

@7iomka
Copy link
Contributor

7iomka commented Mar 8, 2023

What package has an issue

@mantine/core

Describe the bug

Hi.
I plan to migrate to v6 in a near future, but this issue occurred earlier and is still actual in version 6 as well (I attached demo for both versions).

In the example I created a custom component according to the documentation

How you see, we have an type issue, my component receive optional props which can be used in useStyles hook, but, because of nature of createStyles, some of this props are required, so for that case I declared it in defaultProps, but final type is still have error, because type of defaultProps is an Partial from component props.

The one possible workaround for this is to use as const binding without type

const defaultProps = {
  mediaSize: 'md',
  spacing: 15,
} as const;

I saw this workaround you use in your code, but it is not reflected in the documentation in any way. Moreover, in other parts of your code you don't seem to encounter this problem, and I assume it has something to do with the way the types are exported to the end-user module.

I would appreciate your comments on this.

What version of @mantine/hooks page do you have in package.json?

5.10.5

If possible, please include a link to a codesandbox with the reproduced problem

V5 - https://codesandbox.io/s/sweet-nova-1hdww2?file=/src/components/shipment-item/shipment-item.component.tsx:1651-1695

V6 - https://codesandbox.io/s/zen-currying-zj0zbb?file=/src/components/shipment-item/shipment-item.component.tsx:1651-1695

@7iomka 7iomka changed the title useComponentDefaultProps continue to have types conflicts useComponentDefaultProps continue to have types conflicts with optional component props Mar 8, 2023
@rtivital
Copy link
Member

rtivital commented Mar 8, 2023

The types are correct, useComponentDefaultProps returns props type of props, it has no extra logic for detecting what values are defined in defaultProps and I doubt that it is even possible. This works fine in Mantine codebase because we do not use strct mode. It is not planned to change this behavior.

@7iomka 7iomka changed the title useComponentDefaultProps continue to have types conflicts with optional component props [Community help wanted] useComponentDefaultProps types with strict mode in tsconfig? Mar 8, 2023
@7iomka
Copy link
Contributor Author

7iomka commented Mar 8, 2023

Maybe someone can help people like me who use strict: true to find a workaround without manually bindings with as const?
Are there any typescript experts here?
@congyaqwq What do you think? It seems that you have tried to solve this issue

@cyantree
Copy link
Contributor

cyantree commented Mar 8, 2023

I wouldn't say I'm an expert but I'm an advocate for strict mode and therefore confronted with various issues. :D

I've tried to reconstruct your example locally. For me in Mantine v5 even using as const didn't solve the problem as the return type of useComponentDefaultProps() still contains the optional props. However in v6 it solves the issue because the return type has changed.

So I will skip v5 and just provide an alternative for as const:

TypeScript 4.9 introduced the satisfies operator which seems to help here:

const defaultProps = {
    mediaSize: "md",
    spacing: 15,
} satisfies Partial<ShipmentItemProps>;

The result is comparable to as const but with the benefit that the type must be compatible to Partial<ShipmentItemProps>.

Is this good enough for your? For me this would be clean enough because it clearly describes the relation between the props and default props.

@7iomka
Copy link
Contributor Author

7iomka commented Mar 8, 2023

@cyantree thank you, man! It works! I have props suggestion and no errors!

@7iomka 7iomka closed this as completed Mar 8, 2023
@congyaqwq
Copy link
Contributor

I apologize for the inconvenience my PR caused you. What I had in mind was that defaultProps would perform type inference automatically, so there would be no need to write a Partial every time. However, some values can only pass type checking with as const. I also have strict mode enabled in my own project, and I often use as const to remove type errors. The humble person above solved this problem with satisfies, which is really great. Thanks!

@7iomka
Copy link
Contributor Author

7iomka commented Mar 9, 2023

I apologize for the inconvenience my PR caused you. What I had in mind was that defaultProps would perform type inference automatically, so there would be no need to write a Partial every time. However, some values can only pass type checking with as const. I also have strict mode enabled in my own project, and I often use as const to remove type errors. The humble person above solved this problem with satisfies, which is really great. Thanks!

The one problem now (actually for users that uses next.js with babel config): babel configs in next.js not supports satisfies operator now, because babel have unresolved issue and this occurs in next.js projects with babel config.
So, unfortunately, for me is only workaround to use as const - it is not a big deal for this case, but this is kind of at odds with what is described in the documentation and may confuse many new users

@cyantree
Copy link
Contributor

cyantree commented Mar 9, 2023

I think if it should be solved without relying on these things useComponentDefaultProps() should be typed like this:

  • defaultProps may contain some or all keys of props but ideally no more than that
  • The result is a merge of the type of defaultProps and props

I've also discovered that currently it's technically possible to pass some additional props indefaultProps that don't belong to props. In this case TS wouldn't recognize them and throw an error however the result from useComponentDefaultProps() would still include them. So in this case result and types don't match up.

Is there a reason why currently the return type isn't something like T & U? What are special cases that need to be handled differently?
I've seen a filterProps() method that deleted props with value undefined:
https://github.com/mantinedev/mantine/blob/master/src/mantine-styles/src/theme/MantineProvider.tsx#L58
https://github.com/mantinedev/mantine/blob/master/src/mantine-styles/src/theme/utils/filter-props/filter-props.ts#L7
So in this case the result must strip undefined from allowed values but not optional properties as such.

However NonNullable also strips null as accepted value so there might be another hidden issue:
https://github.com/mantinedev/mantine/blob/master/src/mantine-styles/src/theme/MantineProvider.tsx#L51

But TBH I have a hard time understanding these mapped times with utility types and modifiers, so I haven't fully grasped the current return type yet.

Maybe I'll deepdive in the original PR #2065.

@7iomka 7iomka reopened this Mar 9, 2023
@congyaqwq
Copy link
Contributor

I think if it should be solved without relying on these things useComponentDefaultProps() should be typed like this:

  • defaultProps may contain some or all keys of props but ideally no more than that
  • The result is a merge of the type of defaultProps and props

I've also discovered that currently it's technically possible to pass some additional props indefaultProps that don't belong to props. In this case TS wouldn't recognize them and throw an error however the result from useComponentDefaultProps() would still include them. So in this case result and types don't match up.

Is there a reason why currently the return type isn't something like T & U? What are special cases that need to be handled differently? I've seen a filterProps() method that deleted props with value undefined: https://github.com/mantinedev/mantine/blob/master/src/mantine-styles/src/theme/MantineProvider.tsx#L58 https://github.com/mantinedev/mantine/blob/master/src/mantine-styles/src/theme/utils/filter-props/filter-props.ts#L7 So in this case the result must strip undefined from allowed values but not optional properties as such.

However NonNullable also strips null as accepted value so there might be another hidden issue: https://github.com/mantinedev/mantine/blob/master/src/mantine-styles/src/theme/MantineProvider.tsx#L51

But TBH I have a hard time understanding these mapped times with utility types and modifiers, so I haven't fully grasped the current return type yet.

Maybe I'll deepdive in the original PR #2065.

ts playground

I have tried the issue with type checking, but I'm not sure why this phenomenon is different from what I demonstrated in the demo code. I hope this demo will be helpful for you.

@cyantree
Copy link
Contributor

I'm sorry but I think I lost the thread.

So to recap:
The OP had a typing issue in v6 related to defaultProps being types as Partial. This won't work because TS doesn't know which props the partial contains and therefore the resulting type won't have the optional props set.

So to get around this there are currently three possibilities:

  1. Don't add any type information: const defaultProps = {...}; (as shown by @congyaqwq )
  2. Add as const when necessary (e. g. when props contain some constant (union) types like `'s' | 'm' | 'l')
  3. Add satisfies (for TS >= 4.9)

So IMHO the original problem has been solved, correct @7iomka ?

Afterwards we discussed the correctness of the return type as I think it has some flaws. But here I'm not sure if it's useful to tackle that:

  • After thinking about it a bit more the inconsistency when passing unsupported props (TS throws an error, JS passes them through) is ok because TS is working correctly here and if you're ignoring these errors than it's not TS's fault.
  • The optional, NonNullable, filterProps() thing is at this point only a theoretical problem.

So if the original problem is resolved I think this issue can be closed.

What do you all think?

@7iomka
Copy link
Contributor Author

7iomka commented Mar 14, 2023

I will close this, but maybe needed to make a notice about this in the documentation

@7iomka 7iomka closed this as completed Mar 14, 2023
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

No branches or pull requests

4 participants