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

V8: Type error without skipLibCheck #1521

Closed
srapilly opened this issue Jul 17, 2023 · 7 comments
Closed

V8: Type error without skipLibCheck #1521

srapilly opened this issue Jul 17, 2023 · 7 comments
Labels

Comments

@srapilly
Copy link

  • downshift version: 8.0.0
  • node version: 19.9.0
  • npm (or yarn) version: 9.6.3

What happened:

We are using the newest V8 version, thanks for the nice release :)

We have a small issue with the type in our repository, we don't use skipLibCheck and this cause the following type issue:

TS2314: Generic type 'RefObject ' requires 1 type argument(s).

We don't want to use skipLibCheck in our repository because we have some d.ts files that we want to type check and it's not yet possible to only skip the check for library (microsoft/TypeScript#40426)

Reproduction repository:

latest https://github.com/downshift-js/downshift V8 with skipLibCheck: false

Problem description:

Suggested solution:

I tried to do something like this in the type definition,

export interface GetRootPropsOptions<E extends HTMLElement> {
  refKey?: string
  ref?: React.Ref<E>
}

instead of

export interface GetRootPropsOptions {
  refKey?: string
  ref?: React.Ref
}

it seems to work, but seems a bit hard to use at first glance where I could need to do something like this when using a function to get the props:

{...getMenuProps<{ 'aria-labelledby': undefined }, HTMLDivElement>({ 'aria-labelledby': undefined })}
@silviuaavram
Copy link
Collaborator

Hi! Thanks for reporting the issue! Hope we can get it properly fixed soon.

As for the last usage, I don't understand it.

  getMenuProps: <Options>(
    options?: GetMenuPropsOptions & Options,
    otherOptions?: GetPropsCommonOptions,
  ) => Overwrite<GetMenuPropsReturnValue, Options>

getMenuProps has only one generic, and it's not really needed. I think you just pass 'aria-labelledby': undefined to the function and the type will be updated automatically in the return object.

Let me know what you think. Thanks!

@akheron
Copy link

akheron commented Jul 31, 2023

typings/index.d.ts also now unconditionally imports react-native typings. However, @types/react-native is not listed in dependencies, and at least for me, the react-native types are incompatible with some builtin dom types.

import * as ReactNative from 'react-native'

If you need to expose stuff that use react-native typings, you really should be publishing a separate package.

@silviuaavram
Copy link
Collaborator

@akheron it should be listed in the devDependencies.

All things considered, I might just remove its usage, since it's not very important.

Any issue with the original issue in this ticket?

@silviuaavram
Copy link
Collaborator

#1524 <- I think this replacement should be good enough.

@akheron
Copy link

akheron commented Aug 1, 2023

it should be listed in the devDependencies.

Yes, but npm install downshift in my project doesn’t install downshift’s devDependencies.

Removing react-native’s usage from the typings sounds good :)

@silviuaavram
Copy link
Collaborator

@srapilly @akheron do you thing the suggestion from above will solve the issue?

export interface GetRootPropsOptions<E extends HTMLElement> {
  refKey?: string
  ref?: React.Ref<E>
}

As for this concern {...getMenuProps<{ 'aria-labelledby': undefined }, HTMLDivElement>({ 'aria-labelledby': undefined })}, I don't think we support the first generic in our getter props, so it will only be {...getMenuProps<HTMLDivElement>}.

Would there be any relevant cases where the Ref content would not be an extends HTMLElement, so we should chose any instead?

Let's agree upon the best result so we can merge the fix. Thanks!

@silviuaavram
Copy link
Collaborator

Should be fixed by #1536.

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

No branches or pull requests

3 participants