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

loose events back #1398

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RodrigoHamuy
Copy link

@RodrigoHamuy RodrigoHamuy commented Dec 12, 2024

#1145 introduced a breaking change on EventDispatcher, a class with an implementation that hasn't change in years.

The idea is great, but sadly it breaks a few things in pmndrs libraries as it makes backward compatibility really hard to keep.

I did try to fix it at three-stdlib, but I introduced more issues than solutions, also would prefer to avoid having to maintain in there a copy of EventDispatcher considering the real thing hasn't change in years.

I believe a much simpler solution that works for all is to just accept types to be a tiny bit looser.

With this we will still have types for addEventListener and so on, but is optional rather than required.

@Methuselah96
Copy link
Contributor

Do you have a branch in three-stdlib where I can look at some of the difficulties you ran into?

@RodrigoHamuy
Copy link
Author

RodrigoHamuy commented Dec 12, 2024

@Methuselah96

You can check this Codesandbox with [email protected] (before my first attempt at fixing the problem).

https://codesandbox.io/p/sandbox/gf3xzj

import { OrbitControls } from "three-stdlib";
import { PerspectiveCamera } from "three";

const orbit = new OrbitControls(new PerspectiveCamera());
orbit.addEventListener("change", (e) => {
  e.target.object;
});

Throws:

Argument of type 'string' is not assignable to parameter of type 'never'.typescript(2345)

image

@RodrigoHamuy
Copy link
Author

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 this pull request may close these issues.

2 participants