fix: controls event dispatcher types

[RodrigoHamuy]

Fixes #387 by maintaining our own copy of EventDispacher.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[abernier]

hi @RodrigoHamuy, I've just tried upgrading drei to latest three-stdlib but I have TS warnings:

[RodrigoHamuy]

Hi @abernier, really sorry about that.

I probably need some help as I am not sure what should we do. This PR made a copy of EventListener that three-stdlib uses for their controls, as the original one introduced a breaking change in their typings (link) as mentioned in this issue.

But in reality the original EventListener hasn't change in 3 years, only their typings got more restrictive since r168.

My understanding is that because three-stdlib aims at having backwards compatibility, we can't just upgrade to use the latest @types/three, but without doing that all three-stdlib controls event listeners types will be never if you use @types/three > 0.168 in your project. This is what this PR attempted to fix.

Is there another way to do this? I will have a play on my side too, but not really sure we can do anything else apart from actually asking @types/three to revert that change and allow loosely types again ( three-types/three-ts-types#1145 ).

Maybe we should revert this commit in the meantime, sorry for the trouble.

CC @CodyJasonBennett

[CodyJasonBennett]

I believe that type error comes from R3F since now the types diverge. Maybe we can loosen it there.

https://github.com/pmndrs/react-three-fiber/blob/5a5080056fa00e81dba2e1cb922e9875ea545e48/packages/fiber/src/core/store.ts#L112

[abernier]

or couldn't we let r3f uses stdlib's EventDispatcher (for controls rootState type)?

nb: it will add a (dev-)dep to stdlib for fiber

[CodyJasonBennett]

All we need is a small interface that is compatible with both types, although it doesn't matter whether controls emits events or not. It's an implementation detail of three's existing controls, but not user-land. It's possible we can loosen it to a very small interface.

[RodrigoHamuy]

hi @RodrigoHamuy, I've just tried upgrading drei to latest three-stdlib but I have TS warnings: ...

@abernier Is there a way to replicate these errors? I missed this because on my project at least everything is working OK after upgrading. Ignore me, I didn't read that it was on drei itself!

[abernier]

@abernier Is there a way to replicate these errors? I missed this because on my project at least everything is working OK

@RodrigoHamuy yes, if you clone pmndrs/drei @ master and yarn add three-stdlib@latest, then yarn typecheck

you should have the same TS errors

It's possible we can loosen it to a very small interface.

@CodyJasonBennett like this?:

# pmndrs/react-three-fiber/blob/master/packages/fiber/src/core/store.ts

- controls: THREE.EventDispatcher | null;
+ controls: Omit<THREE.EventDispatcher, 'addEventListener' | 'hasEventListener' | "removeEventListener"> | null;

if I understand correctly, this means a user can no longer controls = useThree(state => state.controls); controls.addEventListener(...) without a TS error? -- because, this is considered private methods (sorry I'm just not sure)?

[RodrigoHamuy]

@CodyJasonBennett @abernier here are a few options I think would solve this:

1. My personal favourite, revert those changes from @types/three: loose events back three-types/three-ts-types#1398
2. Otherwise this would work but requires a small change in drei: Fix event dispatcher types #392

I prefer option one, is simple and avoids having to maintain a copy of a class that hasn't changed in years (EventDispatcher) in three-stdlib.

Happy to also show a draft PR of drei changes if interested in option two?

Mainly I believe is just in https://github.com/RodrigoHamuy/drei/blob/master/src/core/OrbitControls.tsx#L7

- export type OrbitControlsChangeEvent = Event & {
-   target: EventTarget & { object: Camera }
- }
+ type ExtractCallback<T, E extends string> = T extends { addEventListener(event: E, callback: infer C): void } ? C : never;
+ export type OrbitControlsChangeEvent = Parameters<ExtractCallback<OrbitControlsImpl, 'change'>>[0]

[tamighi]

I see that the transformControls is a bit more tricky as copying the native three implementation would require to put a lot of transformControls property to public so that it's "transformControlsRoot" has access to them.

https://github.com/mrdoob/three.js/blob/0f523acc5a1d6ae3d4d02a41f5d4793adc9303d5/examples/jsm/controls/TransformControls.js#L763

[RodrigoHamuy]

@CodyJasonBennett @abernier any thoughts on the 2 alternative solutions mentioned here? Neither of them would require changing react-three-fiber. Option 1 (revert @types/three to a more loosen typing) means also revert this PR, which I still think is the easiest, cleanest solution.

[abernier]

iiuc 1. depends on three-types/three-ts-types#1398, whether it is merged or not, is that it?

[abernier]

i mean: it's not possible to revert if three-types/three-ts-types#1398 is not merged, am i right?

[CodyJasonBennett]

Maybe let's open an issue since a revert is unlikely (it is equally breaking to change your mind), and we'd all be interested in a workaround or proper migration. I still think loosening the type on R3F's side would be the best course of action since we don't actually care about controls implementing events at all. It can extend as simple as an opaque object type.

[RodrigoHamuy]

Maybe let's open an issue

@CodyJasonBennett ticket added to R3F: pmndrs/react-three-fiber#3416

Otherwise we can repurpose #387 as the fix was incomplete.

I still think loosening the type on R3F's side would be the best course of action since we don't actually care about controls implementing events at all. It can extend as simple as an opaque object type.

Are you suggesting this? I'm worried bellow change will introduce a breaking changes for anybody that expects EventDispatcher api to be available. Maybe one for v9 instead?

// file: https://github.com/pmndrs/react-three-fiber/blob/master/packages/fiber/src/core/store.ts#L112
- controls: THREE.EventDispatcher | null
+ controls: {} | null

Personally, I think that if reverting both this PR and @types/three breaking changes (three-types/three-ts-types#1398) are not an option, the next less intrusive solution is this instead: #392

[CodyJasonBennett]

I meant either an opaque type that implemented event listener methods without generics or an object like that. This still might be too strict since people may implement their own interface with parameters; again, I think controls both shouldn't be in R3F to begin with, and the type being an irrelevant and inconsistent constraint—assuming that all controls follow three/examples, which is certainly not the case.

// file: https://github.com/pmndrs/react-three-fiber/blob/master/packages/fiber/src/core/store.ts#L112
- controls: THREE.EventDispatcher | null
+ controls: {
+   addEventListener(type: string, listener: (...args: any[]) => void): void
+   hasEventListener(type: string, listener: (...args: any[]) => void): boolean
+   removeEventListener(type: string, listener: (...args: any[]) => void): void
+   dispatchEvent(event: any): void
+} | null

If #392 can create a compatible overload, then let's roll with that. Still, I'm unhappy with R3F taking such opinions over user code.

[RodrigoHamuy]

@CodyJasonBennett fair enough. In that case I leave it with you to decide between these (I didn't add the loosing types of the R3F store because I'm worried it will snowball this bug even more):

• option A
  Fix event dispatcher types #392
  +
  fix DREI to work with latest three-sdlib drei#2276
• option B
  loose events back three-types/three-ts-types#1398
  +
  revert fix: controls event dispatcher types #388

My personal preference is still option b since it reduces bundle size (we don't need to keep our copy of EventDistpacher), we stay more closely aligned with three/examples, and (unless people updated very very recently) it means no changes at all for them or our APIs.