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

Align onXYZ method signatures to match event payloads #3106

Open
mattjennings opened this issue Jun 20, 2024 · 8 comments
Open

Align onXYZ method signatures to match event payloads #3106

mattjennings opened this issue Jun 20, 2024 · 8 comments
Labels
api change Breaking api change

Comments

@mattjennings
Copy link
Contributor

Context

Actors (and maybe other classes) expose methods such as onPreUpdate or onCollisionStart as convenient handlers for events to avoid manually wiring up to events.

However, the method signature is different than what you would get if you did manually wire up to the event.

// method on Actor
class Player extends ex.Actor {
   onPreUpdate(engine: Engine, delta: number) {}
}

// wiring up to event manually e.g. when extending Entity
class Player extends ex.Entity {
   constructor() {
     super()

     this.on('preupdate', this.onPreUpdate)
   }

   onPreUpdate({ engine, delta }: ex.PreUpdateEvent) {}
}

This makes it inconvenient if I want to move code around in such a way that i'm going to/from an event handler/class method.

Proposal

This is a severely breaking change

Align all onXYZ convenience methods to use event as the parameter rather than multiple parameters from the event.

class Player extends ex.Actor {
   onPreUpdate({ engine, delta }: ex.PreUpdateEvent) {}
}
@mattjennings
Copy link
Contributor Author

we could consider writing a codemod to help with the breaking change

Copy link

This issue hasn't had any recent activity lately and is being marked as stale automatically.

@github-actions github-actions bot added the stale This issue or PR has not had any activity recently label Aug 20, 2024
@Autsider666
Copy link
Contributor

Playing devil's advocate: What about deprecating and later removing the hardcoded onXYZ functions altogether? The added value of these, for as far as I could see separately implemented most of the time, onXYZ functions seems a bit low with on already existing. A dev could extend a class to create the same onXYZ functions with it if they prefer using them.

@mattjennings
Copy link
Contributor Author

They should definitely stay. There's a very high chance your extended actor will need to use one of those events, so it removes boilerplate by having these. It's also a familiar concept for devs coming from Unity etc.

@Autsider666
Copy link
Contributor

Ah, good to know. I'm not familiar with Unity at all, so that explains their usage.
Does this (or some other industry standard) include the custom code used for some of the existing onXYZ functions (example) instead of using on in the background?

@mattjennings
Copy link
Contributor Author

I could be wrong but I'm guessing it's done that way to save setting up an event listener unnecessarily in the case that the extended actor doesn't need it.

@github-actions github-actions bot removed the stale This issue or PR has not had any activity recently label Aug 28, 2024
@eonarheim eonarheim added the api change Breaking api change label Oct 6, 2024
@eonarheim
Copy link
Member

@mattjennings How do you feel if we defer this a release to v0.31 or RC (whichever is first?)

@mattjennings
Copy link
Contributor Author

yup works for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change Breaking api change
Projects
None yet
Development

No branches or pull requests

3 participants