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

[WIP] Built-in events rework #93

Closed
LeonMrBonnie opened this issue Dec 1, 2021 · 3 comments
Closed

[WIP] Built-in events rework #93

LeonMrBonnie opened this issue Dec 1, 2021 · 3 comments
Labels
scope: shared Both module related issues type: enhancement New feature or request

Comments

@LeonMrBonnie
Copy link
Contributor

Description of the problem

The current system of handling built-in alt:V events is prone to issues and name collisions because the system just registers these events under a pre-defined name.
To work around this issue, alt.on should ONLY be for custom events, and the built-in events are handled in some other way, for example something like this alt.onPlayerConnect(() => {}) (This idea is still being worked on)

Also, the event arguments are passed individually to the function, which means: Its impossible to add new parameters to somewhere except the end, because it otherwise breaks all existing code.
Instead the event arguments of built-in events should be passed to the function as a single object containing all the event data. This may seem like it would complicate things, but because we are using modern JavaScript, we can use e.g. the object destructuring syntax, allowing code to look almost the same as before, but being a lot more flexible.

By doing this we will have to completely seperate the handling of built-in and custom events in the module, which will on one hand be a big change to the codebase, but on the other hand will also make the code more understandable (Seriously, try understanding how events currently work in the module by just looking at the source, it's all over the place) and easier to maintain.

When we first implement this new system, we will have to keep the old event system as it is but completely deprecate it for AT LEAST 2 release versions, because this change is breaking probably every script that was ever written for alt:V. Hell, maybe we are going to keep the old system forever just for compatibility sake, but this is something that has to be properly thought out once the implementation is finished and the new events system has been thoroughly tested.

Desired solution for the problem

Proposed typings:

// wip

Alternatives you considered

No response

Additional context

No response

Version

No response

Scope

shared

@LeonMrBonnie LeonMrBonnie added type: enhancement New feature or request scope: shared Both module related issues labels Dec 1, 2021
@LeonMrBonnie LeonMrBonnie changed the title Built-in events rework [WIP] Built-in events rework Dec 1, 2021
@zziger
Copy link
Contributor

zziger commented Dec 1, 2021

Tbh i find possibility to register to a multiple events using one method (alt.on currently) quite useful, as it can be used in multiple situations e.g. subscribing to events using decorators, in the way that doesn't require you to duplicate your code. If the main problem of the alt.on are name collisions with the custom events maybe it's worth considering using something like Symbol for that reason?
For example: alt.on(alt.events.playerConnect).
Or, maybe those methods can be separated to their own method like alt.onAltEvent (here should be some better name, but i guess that describes what I mean nicely), so it will still be possible to register an event like a normal event, by its some identificator?

@LeonMrBonnie
Copy link
Contributor Author

I think the idea with using Symbols for the built-in events is a good idea, and would also make the API at least a bit backwards compatible because the actual function doesn't change.

@LeonMrBonnie
Copy link
Contributor Author

See #230

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: shared Both module related issues type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants