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

Types of disableHeight and disableWidth are incorrect #70

Closed
samtjo opened this issue Jul 17, 2023 · 6 comments
Closed

Types of disableHeight and disableWidth are incorrect #70

samtjo opened this issue Jul 17, 2023 · 6 comments

Comments

@samtjo
Copy link

samtjo commented Jul 17, 2023

I believe the types of the disableHeight and disableWidth props are incorrectly set to false here etc - when they should actually be boolean.

It leads to TypeScript errors like in the screenshot when trying to use them:

image

It looks like this was introduced in 1.0.18 if that helps.

Thanks!

@64BitAsura
Copy link

+1

@bvaughn
Copy link
Owner

bvaughn commented Jul 17, 2023

The types were chosen very intentionally to prevent bad combinations of params (e.g. an onResize callback that tries to access height even though the component has been configured to disableHeight)

I chose not to support a resizer that had both height and width disabled because that seems pointless. If you'd like to make an argument for why that should be supported, I'll consider it.

@samtjo
Copy link
Author

samtjo commented Jul 17, 2023

I think my main issue with the current approach, is that it's now impossible to programmatically toggle disableHeight or disableWidth with a boolean, i.e.

let toggle: boolean = false;

if (some condition) toggle = true;

return (
   <AutoSizer disableHeight={toggle}>
   // Type 'boolean' is not assignable to type 'false'.

I would argue just set these props as booleans, and leave it to the user to determine which combination of height and width disabled they want. There can be use cases like ours, where we do sometimes need both to be disabled, depending on the layout we're going for.

Also your documentation states that both these props are booleans, which (going on TypeScript types at least) isn't right. If you decide not to change this, can you at least update the docs to make it clear how this works without having to dig into the code please?

@bvaughn
Copy link
Owner

bvaughn commented Jul 17, 2023

They are booleans but that does not mean that all props combinations are valid. For example, this would not be valid and is something TypeScript should warn about:

<AutoSizer
  disableHeight={toggle}
  onResize={({ height }) => {
    // ...
  }
/>

I would argue just set these props as booleans

This caused confusion before (or maybe more accurately, it didn't catch incorrect combinations like the one I mentioned above) and people complained about it, so now the types are more restrictive.

If this is a problem for your specific case, you can always tell TypeScript to ignore (and/or cast the type).

@samtjo
Copy link
Author

samtjo commented Jul 18, 2023

Ok thanks for explaining and getting back to me so quickly @bvaughn. I'll just tell TypeScript to ignore for our use case and close this issue!

@samtjo samtjo closed this as not planned Won't fix, can't repro, duplicate, stale Jul 18, 2023
@bvaughn
Copy link
Owner

bvaughn commented Jul 18, 2023

No problem 😃

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

Successfully merging a pull request may close this issue.

3 participants