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

Interface 'IMidiPermissionDescriptor' incorrectly extends interface 'PermissionDescriptor' #2092

Open
zhangshichuan opened this issue Sep 1, 2021 · 16 comments · May be fixed by #2101
Open

Comments

@zhangshichuan
Copy link

What is the current behavior?
image
image
image
image

Steps to reproduce it and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have extra dependencies other than react-use. Paste the link to your JSFiddle or CodeSandbox example below:

What is the expected behavior?

A little about versions:

  • OS:
  • Browser (vendor and version):
  • React:
  • react-use:
  • Did this worked in the previous package version?
@kachkaev
Copy link
Contributor

kachkaev commented Sep 1, 2021

We're also seeing this in react-use 17.3.1 alongside typescript 4.4.2. Linting passes because CI still uses typescript 4.1.1. The error comes from:

interface IMidiPermissionDescriptor extends PermissionDescriptor {
name: 'midi';
sysex?: boolean;
}
interface IDevicePermissionDescriptor extends PermissionDescriptor {
name: 'camera' | 'microphone' | 'speaker';
deviceId?: string;
}

That points us to #2063. @gelove what would be the right solution? It’s strange that lib.dom.d.ts sets

type PermissionName = "gamepad" | "geolocation" | "notifications" | "persistent-storage" | "push" | "screen-wake-lock";

That excludes "midi" etc. Not sure how to solve the typings apart from:

interface IMidiPermissionDescriptor extends Omit<PermissionDescriptor, "name"> {
    name: 'midi';
    sysex?: boolean;
}
interface IDevicePermissionDescriptor extends Omit<PermissionDescriptor, "name"> {
    name: 'camera' | 'microphone' | 'speaker';
    deviceId?: string;
}

Would that be semantically correct?

@scvnathan
Copy link

I've tested this out with TS 4.4 and this works too: https://gist.github.com/scvnathan/e9ff3a784fda80b408f514ab6f3acc68

I'm guessing these permission types are experimental so the TS team removed them

@kachkaev
Copy link
Contributor

kachkaev commented Sep 1, 2021

@scvnathan sounds reasonable! Would you be willing to submit a PR? 👍

We can place type PermissionDesc = after interface Experimental...Descriptor definitions and possibly keep the original name (IPermissionDescriptor) to minimise the diff. TBH I prefer unabbreviated names without I/T prefix like ExtendedPermissionDescriptor, but we can leave this for later 🙂

@scvnathan
Copy link

Sure, I can submit a PR 👍

@scvnathan scvnathan linked a pull request Sep 1, 2021 that will close this issue
13 tasks
@JoeDuncko
Copy link

Hi! @react-hookz/web, the new library by one of react-use's former maintainers (background here and here) has resolved this outstanding type issue. It might be worth taking a look.

For those interested, there's an official migration guide for migrating from react-use to @react-hookz/web.

@leepowelldev
Copy link

Is this library offically unmaintained now? This bug is blocking our build pipelines - we can put in workarounds for now but it's not ideal.

@JoeDuncko
Copy link

@leepowelldev Unfortunately this library is not "officially unmaintained", as no one has been able to establish contact with @streamich (the project owner). As far as I can tell, every couple months he appears, accepts a PR or two, then disappears again. Personally, I tried contacting him multiple times to see if I could help with the project backlog. No luck. So I'd say this project is "for all intents and purposes, unmaintained".

@leepowelldev
Copy link

@JoeDuncko Sure. Kind of annoying as I'd happily move to @react-hookz/web, but the reality is many of the hooks we use in this library are not yet implemented.

@JoeDuncko
Copy link

JoeDuncko commented Sep 5, 2021

If you compile a list of react-use hooks you use that are not yet in @react-hookz/web and send it my way? It'd help us figure out what hooks we should be focusing on next. Thanks!

@leepowelldev
Copy link

@JoeDuncko - thanks for this, just gone through and seems the only one we're missing that would allow us to migrate is useWindowSize, if I have time over the next day or two I can pull together a PR for this unless you already have one in the works?

@JoeDuncko
Copy link

@JoeDuncko - thanks for this, just gone through and seems the only one we're missing that would allow us to migrate is useWindowSize, if I have time over the next day or two I can pull together a PR for this unless you already have one in the works?

Awesome! I don't think we have it actively in the works, so a PR would be appreciated!

@Rey-Wang
Copy link

@kachkaev Hello, Is there a plan to merge the PR and release a new version?

@kachkaev
Copy link
Contributor

kachkaev commented Oct 3, 2021

@Rey-Wang I believe that #2101 should fix this issue. I am able to merge the PR, but only @streamich can publish a new version of react-use on npm. Looks like we’ll need to wait for his help here.

@jamesthomsondev
Copy link

@leepowelldev Unfortunately this library is not "officially unmaintained", as no one has been able to establish contact with @streamich (the project owner). As far as I can tell, every couple months he appears, accepts a PR or two, then disappears again. Personally, I tried contacting him multiple times to see if I could help with the project backlog. No luck. So I'd say this project is "for all intents and purposes, unmaintained".

I just started using this lib and ran into this build problem (annoyingly I don't even use usePermission, but it still won't build because of it). Disappointing that such a great lib is being thrown away due to inactive maintenance. GitHub should introduce some sort "claim abandoned repo" feature (yes, I know you can fork but it's not really the same). Ah well, guess I'll have to switch to @react-hookz/web...

@sidhuko
Copy link

sidhuko commented Nov 16, 2021

As a workaround to this you can change your imports to direct imports:

// @deprecated author doesn't maintain react-use actively.
// use @react-hookz/web instead
import useKey from 'react-use/lib/useKey';
import useDebounce from 'react-use/lib/useDebounce';
import useClickAway from 'react-use/lib/useClickAway';
import useTimeoutFn from 'react-use/lib/useTimeoutFn';
import useEffectOnce from 'react-use/lib/useEffectOnce';
import useWindowSize from 'react-use/lib/useWindowSize';
import usePreviousDistinct from 'react-use/lib/usePreviousDistinct';

export { useKey, useDebounce, useClickAway, useTimeoutFn, useEffectOnce, useWindowSize, usePreviousDistinct };

As long as you're not using usePermission you can stop the build issue. You can migrate to new library as they become available or start to write your own replacements.

@Suzan-Dev
Copy link

As a workaround to this you can change your imports to direct imports:

// @deprecated author doesn't maintain react-use actively.
// use @react-hookz/web instead
import useKey from 'react-use/lib/useKey';
import useDebounce from 'react-use/lib/useDebounce';
import useClickAway from 'react-use/lib/useClickAway';
import useTimeoutFn from 'react-use/lib/useTimeoutFn';
import useEffectOnce from 'react-use/lib/useEffectOnce';
import useWindowSize from 'react-use/lib/useWindowSize';
import usePreviousDistinct from 'react-use/lib/usePreviousDistinct';

export { useKey, useDebounce, useClickAway, useTimeoutFn, useEffectOnce, useWindowSize, usePreviousDistinct };

As long as you're not using usePermission you can stop the build issue. You can migrate to new library as they become available or start to write your own replacements.

Thanks!

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.

10 participants