WICG / webhid

Web API for accessing Human Interface Devices (HID)
Other
140 stars 35 forks source link

HIDDevice class - problem using "this" as named parameter in method declarations #77

Open davisford opened 3 years ago

davisford commented 3 years ago

This is a TypeScript project bootstrapped with create-react-app, here are my imports:

├── @material-ui/core@4.12.3
├── @material-ui/icons@4.11.2
├── @testing-library/jest-dom@5.14.1
├── @testing-library/react@12.0.0
├── @testing-library/user-event@13.2.1
├── @types/jest@27.0.1
├── @types/node@16.7.13
├── @types/react-dom@17.0.9
├── @types/react-router-dom@5.1.8
├── @types/react@17.0.20
├── @types/w3c-web-hid@1.0.2
├── dukpt@3.0.0
├── react-dom@17.0.2
├── react-router-dom@5.3.0
├── react-scripts@4.0.3
├── react@17.0.2
├── typescript@4.4.2
└── web-vitals@2.1.0

Here is my tsconfig.json

{
  "compilerOptions": {
    "target": "es5",
    "lib": [
      "dom",
      "dom.iterable",
      "esnext"
    ],
    "allowJs": true,
    "skipLibCheck": true,
    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "noFallthroughCasesInSwitch": true,
    "module": "esnext",
    "moduleResolution": "node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "jsx": "react-jsx"
  },
  "include": [
    "src"
  ],
  "files": [
    "src/typings/index.d.ts"
  ]
}

The issue appears to be the declaration of the oninputreport function on the HIDDevice class as such:

/*~ https://wicg.github.io/webhid/#hiddevice-interface */
declare class HIDDevice extends EventTarget {
    oninputreport: ((this: this, ev: HIDInputReportEvent) => any) | null;
    // etc
}

Using this here seems to constrain the function to force it to be implemented either on HIDDevice or else if implemented on another class it forces it to use this as the parameter name, which causes other problems...here's an example of what I mean:

Screen Shot 2021-09-07 at 10 03 45 AM

Note above the TS compiler is complaining that my implementation of that function is not assignable b/c the parameter name is not matching. I attempted to underscore it, but it still does not like it:

Screen Shot 2021-09-07 at 10 04 03 AM

So, I tried to override it using a typings file, but that still isn't working. The only hack workaround at this point is to cast it to unknown and then cast it back which is realistically a very poor hack.

Screen Shot 2021-09-07 at 10 09 24 AM

Of course, if I name the parameter this in my function implementation, well this causes me other issues:

Screen Shot 2021-09-07 at 10 18 03 AM

I can no longer use this as intended on my own class.

The fix here seems fairly simple. In the spec / implementation of the typings, you could re-do the declaration like so:

/*~ https://wicg.github.io/webhid/#hiddevice-interface */
declare class HIDDevice extends EventTarget {
    oninputreport: ((dev: HIDDevice, ev: HIDInputReportEvent) => any) | null;
    // etc
}

I see several other areas of the typings spec where this is used quite a bit. I would advise against this if possible.

davisford commented 3 years ago

Just out of curiosity, is there a specific reason to have the first parameter to the oninputreport method at all? The HIDInputReportEvent object itself has a reference to the HIDDevice, so it seems redundant / un-necessary.

cmawhorter commented 2 years ago

this has special meaning in typescript.

additionally, oninputreport is a property and not a method. you'd probably want to do MySubclass: ctor { this.addEventListener('inputreport', this._onInputReport.bind(this)) }