Support Pixi 7 events

[zOadT]

Hey!

Pixi.js 7.0.0 finally got released, so I thought about how we can support it in this project. So far I experimented with the typings and wanted to ask you for some feedback:

The Interaction package got replaced by the events package in version 7, which has way better typings for the EventEmitters. Therefore it is now possible to derive the event names and callback parameters directly from Pixi instead of hardcoding them.

Notice that I put a on prefix on all event listeners (so e.g. click becomes onclick). The motivation was that this gives us an easy way to distinguish event handlers from ordinary props in the runtime (so we can correctly assign them as event handlers instead of just assigning them like all other properties). Another benefit is that it is closer to what react-dom does (onClick). (BTW: we can also do camelCase in the typings, it would just be an additional .toLowerCase call at runtime). Technically it is a breaking change, but it only applies when you update to Pixi.js 7 and then you have to adapt your handlers anyways.

But I wanted to ask for your opinion. We can also adapt the typings without the on prefix and have a hard coded list of event names or use some other technique. Depending on your feedback I would implement the runtime accordingly.

Notes:

• No runtime code change so far, but this will also be part of this PR
• For Pixi.js <= 6 the typings are exactly the same as before
• This requires Typescript 4.1, but we can use types versions to make this backwards compatible
• Currently I just use the PIXI.DisplayObjectEvents type, but this can probably be improved. Haven't tested yet if this typing correctly takes the events from the component

  type Test<T> = T extends EventEmitter<infer EventTypes extends PIXI.DisplayObjectEvents>
  ? {
  [P in EventEmitter.EventNames<EventTypes>
    as P extends string ? `on${P}` : never
  ]?: EventEmitter.EventListener<EventTypes, P>
  }
  : never

• Pixi.js 7 also added support for *capture events (e.g. clickcapture). They don't show up in their typings and therefore are also not present in our typings. But I think this is a bug in Pixi.js (but will check this and will open a PR if necessary)

[codesandbox[bot]]

  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

[codesandbox-ci[bot]]

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.

Latest deployment of this branch, based on commit 1e55199838af60a75c7fc1edcd7379f2ca5d3667:

Sandbox	Source
React	Configuration
React Typescript	Configuration
examples	Configuration
React Pixi Fiber template	Configuration
React Pixi Fiber TypeScript template	Configuration
React Pixi Fiber demo – Rotating Bunny	Configuration
React Pixi Fiber demo – AnimatedSprite	Configuration
react-pixi-fiber text alignment	Configuration
React Pixi Fiber with Redux	Configuration
React Pixi Fiber demo – Animated.ValueXY	Configuration

[michalochman]

Hi @zOadT, thanks for pushing it. Since it's just an update for types, let me comment on your notes below.

For Pixi.js <= 6 the typings are exactly the same as before

cool

This requires Typescript 4.1, but we can use types versions to make this backwards compatible

This repo has TypeScript 4.3.5 in devDependencies, so I think 4.1 should be safe.

Pixi.js 7 also added support for *capture events (e.g. clickcapture). They don't show up in their typings and therefore are also not present in our typings. But I think this is a bug in Pixi.js (but will check this and will open a PR if necessary)

Can you ask about this in pixi.js repo?

[michalochman]

Another note with regards to maintaining backwards compatibility with previous PixiJS versions. I have some ideas:

Maintain separate createInstance per major PixiJS version.

👍 Library is easy to use if you only want to use built-in components. 👎 Library is difficult to maintain as it needs to track updates to PixiJS.

Remove built-in createInstance function and write built-in components like Custom Components

👎 Library is difficult to use – breaking changes are introduced and users need to decide how instances are created and how props are handled, could introduce bugs. 👍 Library would be easy to maintain, changes are rarely needed.

---

Ultimately I would like to combine both to keep 👍 and minimize 👎 , i.e.:

• Do not differentiate built-in and custom components – both should use the same API, e.g. combine createInstance and CustomPIXIComponent into createPixiComponent.
• Maintain library of PIXI components for each major (or major.minor) PixiJS version, e.g. instead of

  import { Container } from 'react-pixi-fiber'

  do

  import { Container } from 'react-pixi-fiber/components/pixi-7.0'

• Rewrite components to TypeScript so it's easier to maintain typings for them.

---

This is a large project and I'm not sure when I would be able to commit to it.

[zOadT]

Thank you very much for your comments! The issues with Pixi.js are already solved in https://github.com/pixijs/pixijs/pull/8828.

Concerning backwards compatibility: I don't think this is an issue for this PR right now because you probably only have to update something around setValueForProperty for the update to v7, but I generally understand that this is an issue. I got some thoughts on that, but I would first try to implement the runtime change in the next 2-3 days so that I have seen the code a bit more. After that we can look at how to properly incorporate the changes 🙂

[zOadT]

Hey! The binding of the event listeners works so far, but maybe you could help me by two points: (BTW, no worries, I will fix and extend the tests)

1. Currently the functionality is implemented by a special rule directly in setValueForProperty. I want to embed the changes more properly with a new property type LISTENER, but this would lead to special rules in getPropertyInfo and getDefaultValue and makes it less obvious, that listeners are handled slightly different (they have to be removed on invalid values). Do you have some general thoughts about that? Also do you think checking for startsWith('on') is ok or do you prefer having a fixed list of property names?

2. There is an edge case: Say the user renders a component with e.g. onmousedown={cb} and also additionally manually binds the event listener using a ref ref.current.on('mousedown', cb), so that cb is bounded twice. Now, if the component gets rerendered with another value (e.g. onmousedown={null}) both bindings of cb get removed. You can work around that by doing something like:

   const listenerMap = new WeakMap();
   
   function addPixiCallback(instance, eventName, listener) {
     let l = listenerMap.get(listener);
     if (typeof l === "undefined") {
       l = function reactPixiFiberCallback(...args) {
       return listener.apply(this, args);
       };
       listenerMap.set(listener, l);
     }
     instance.on(eventName, l);
   }
   
   function removePixiCallback(instance, eventName, listener) {
     const l = listenerMap.get(listener);
     instance.off(eventName, l);
   }
   
   export function replacePixiCallback(instance, eventName, listener, prevListener) {
     if (typeof prevListener === "function") {
       removePixiCallback(instance, eventName, prevListener);
     }
     if (typeof listener === "function") {
       addPixiCallback(instance, eventName, listener);
     }
   }

   I feel like this isn't really worth the added complexity, but I'm not sure. What do you think?

[zOadT]

Update: It seems like the old event handling could return in Pixi.js 7.1! (https://github.com/pixijs/pixijs/issues/8837) This would make things way more easy, so I guess we should just wait how this turns out

[michalochman]

Thanks @zOadT.

1. Can you check how event listener props are handled by react-dom? Current implementation of property info was based on react-dom, so we would prefer to handle them in a way similar to react-dom.
2. Setting event listeners with props and with ref seems strange so I would just ignore it and mention somewhere in docs that this is forbidden. Unless it is handled nicely in react-dom, then we could mirror that.

I hope the old behaviour is brought back to PixiJS, because it is extremely easy to use with react props and sabes us a headache of managing references to callbacks!

[zOadT]

Replaced by #306