Event handlers .on and .unsubscribe/.unsubscribeAll are inconsistent

[Kusmeroglu]

Description

The event handlers for the Rive class are inconsistent in their naming. Usually addEventListener/removeEventListener (javascript DOM API) or .on/.off (popularized by jQuery) or .subscribe/.unsubscribe are used. It's confusing to have Rive.on/Rive.unsubscribe because the names are not obviously related. I also can't find anything in the Rive API docs regarding these methods so it was hard to discover that they existed.

These functions are defined here for .on and here for .unsubscribe.

I am glad to provide a pull request to fix this!

Provide a Repro

I can't use the onStateChange argument to the Rive constructor because my handleStateChange function changes during the lifetime of the Rive, so my usage looks like this:

useEffect(() => {
    rive?.on('statechange' as EventType, handleStateChange);
    return () => {
      rive?.unsubscribe('statechange' as EventType, handleStateChange);
    };
  }, [rive, handleStateChange]);

Expected behavior

Both Rive.on and Rive.off exist and are aliases to Rive.subscribe and Rive.unsubscribe (or vice-versa).

[zplata]

Thanks for the detailed description @Kusmeroglu! You're right, it does look like we have an inconsistent naming pattern here. We're always happy to take in pull requests if you'd like to add the change! I think on/off makes sense as it also aligns with the direct callbacks you can attach to rive (i.e., onStateChange), so it may make sense to add an off alias to unsubscribe and perhaps add a deprecation notice that unsubscribe would be removed in a future major version bump. That way we can make this a minor/patch version bump for now.

Additionally, when this change goes in, we'll make sure to reflect the docs.

[Kusmeroglu]

@zplata I put up a PR here: https://github.com/rive-app/rive-wasm/pull/328 There is an 'unsubscribeAll', and I'm not sure what name would match on/off. Perhaps off with no callback supplied, or removeAllListeners(type).

[zplata]

@Kusmeroglu thanks for this! We'll get this patched and pushed in next chance we get. I think something like the latter makes sense for the unsubscribeAll - removeAllEventListeners or something. We can add a separate commit for that

[zplata]

Released in v1.2.3