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

API rework proposal #230

Closed
LeonMrBonnie opened this issue Nov 27, 2022 · 22 comments
Closed

API rework proposal #230

LeonMrBonnie opened this issue Nov 27, 2022 · 22 comments
Labels
scope: shared Both module related issues type: enhancement New feature or request

Comments

@LeonMrBonnie
Copy link
Contributor

LeonMrBonnie commented Nov 27, 2022

Description of the problem

The current API is inconsistent, clunky and full of hindsight errors, so it is time we rework the API.

Desired solution for the problem

Table of Contents

General principles

In general, the API will stay close to what it is right now. We don't want to do giant changes that will cause people to adapt to a whole new architecture.

Instead, we want to take the current API and leave everything that works well as it is, remove stuff that is bad and replace it with better stuff. Also, we want to add new stuff that makes working with the API easier (e.g. Entity meta), but which is optional to use.

The new API should still feel close to the old API, but with a breath of fresh air and less questionable design decisions.

Most importantly the new API will feature something the old API never did; CONSISTENCY.

For the new API we will create guidelines for naming and general design, so that it does not end like a inconsistent mess like the current API.

Everything proposed here is just a proposal and nothing completely decided on, so if you are reading through this and find something you disagree with, please leave a comment explaining what you would change and your reasoning behind it.

Factory instead of constructor

In the current API, all alt:V entities created from user code are created via the constructor.

So the user calls const vehicle = new alt.Vehicle(...) and creates a new vehicle on the server.

Instead of allowing this use of constructors, we will instead use factories for creating alt:V entities from user code.

Instead of calling the constructor, the user will call a factory like this: const vehicle = alt.Vehicle.create(...)

This makes it more clear that the vehicle is not owned by the script itself, but by the alt:V core layer itself.

(And also this is a limitation that v8pp gives us... but that's not the actual reason for this proposal)

Entity meta

There are several types of meta data available, e.g. synced meta, stream synced meta and so on. In the current API you access them by using the .set*Meta(key, value) and .get*Meta(key) methods.

To make this a bit easier to use, the new API will provide .*meta properties for reading and writing of these meta types. For example, instead of using .setSyncedMeta("test", 123) you will be able to just use .syncedMeta.test = 123

This is just a small addition, but something that makes the API a bit nicer to use. (And also resembles .data from RageMP, which makes switching from there to alt:V easier)

Events

The event system will receive the biggest rework in the new API and will not follow the "change it but don't change it too much" approach.

Right now events are catched via alt.on(eventName, handler) which is an API that will still exist, but that API will only catch custom events, and no alt:V built-in events anymore.

Instead, the built-in events can be listened to by calling e.g. alt.onPlayerConnect(handler), which just solves a hundred headaches caused by the shared event system for built-in and custom events.

Not only will we move built-in events to a different API, but the arguments received in events will also be reworked. Currently, you receive the event arguments as normal function arguments, this has the downside that it has bad backwards compatibility, passing functions sucks and its hard to maintain.

The new event API instead will pass only one argument to the event handler, which is the event context.

The event context is an object that contains all the data of the event, and also in some cases additional data such as functions that can be called.

To get a feel how the new event API will work, lets look at this imaginary event here:

alt.onImaginaryEvent(({ player, weapon, cancel }) => {
    alt.log(`${player.name} called the imaginary event with weapon ${weapon}`);
    if(weapon === "weapon_pistol") cancel();
});

As we can see there, we use the JS object destructuring feature, which makes this a lot easier to read.
Alternatively, we could also write it like this: (But that is not recommended)

alt.onImaginaryEvent((ctx) => {
    alt.log(`${ctx.player.name} called the imaginary event with weapon ${ctx.weapon}`);
    if(ctx.weapon === "weapon_pistol") ctx.cancel();
});

This object approach makes adding new event arguments easier (better backwards compatibility), lets users choose what arguments they need and in what order they need. And additionally as previously mentioned, also allows us to attach some functions to an event.

The return values of events will be completely ignored, in the current API it is used to cancel an event, which will be replaced by the .cancel() function of the event context. This also allows us to add arguments to the .cancel() method for e.g. specifying a reason.

The event context is also used for custom events sent by the scripts themselves, but without the additional .cancel() function attached to it.

Entity specific events

Before reading this make sure to read the Events section

There are many events attached to an entity directly, e.g. playerConnect would be one of those events, so to make it easier in some situations to catch an event only for a specific entity, we will provide an API that catches that event only for that specific entity.

As an example, lets use the playerWeaponChange event here:

player.onWeaponChange(({ oldWeapon, newWeapon }) => {
    alt.log(`${this.name} switched their weapon from ${oldWeapon} to ${newWeapon}`);
});

The this context of the event handler passed here, will always be the entity itself.

Using the new API

To preserve backwards compatibility we have to make sure that this new API and the old API are both accessible, so to make this new API possible we will have to use a new import name for the API.

The old API was (and will be) accessible by importing alt-shared, alt-client and alt-server.

We will move the new API to a new import, to make a clear distinction between the legacy and new API;
It will use @altv/shared, @altv/client and @altv/server for importing the new API.

The old imports will continue to work, but the legacy API will not receive any more updates, so all new features will only be available via the new API. And there will be no guarantees for the stability of the legacy API.

Lazy properties

(Internal, this section is related to the inner workings of the module instead of the API itself)

Right now all properties provided by the API are normal getters that call C++ code, to get the current value from the SDK.

This approach is very wasteful, because there are quite a few properties which do not need to be called more than once. For example the .id property on entities is a) readonly and b) never changes for the lifetime of the entity. So calling this getter every time the user wants to access it, is wasted computation time.

Instead, the new API will use lazy getters which can always be used for properties that are readonly and never change. Lazy getters are only called once per object and the result of it is then stored directly in JS, so the next time the user accesses this property, it's like looking up a normal JS object property. This is much faster than calling into C++ code that calls the SDK getter.

Binding creation

(Internal, this section is related to the inner workings of the module instead of the API itself)

Currently, we create the bindings mostly manually, with some macros we wrote as helpers for converting JS values to C++ values etc.

This approach sucks. It's a pain to maintain and adding new APIs takes way too much time, when there are libraries like v8pp out there, that make binding C++ to JS much easier.

So for this API rework we will also try out v8pp for the bindings, and if it works out well use it for most (if not all) of the bindings in the reworked API.

Shared bindings

(Internal, this section is related to the inner workings of the module instead of the API itself)

While we are at it and reworking the API, we can also fix something that has been causing a headache in writing the bindings for a long time; Shared bindings.

In the current API there are shared bindings, but only for whole classes / namespaces and not for parts of classes. As an example, lets look at the Player class. It's available on both sides, but only a part of the API the Player class has is available on both sides. Right now, this API that is available on both sides is just replicated on both sides, which have their own Player class. Instead of doing this, we will have a SharedPlayer class (that is not accessible from JS code) which is in the shared side of the module, and the individual Player classes will inherit from this class.

This shared class will only have that part of the API that is actually, shared and the inherited classes implement the other parts of the API that are available for that respective side.

@LeonMrBonnie LeonMrBonnie added type: enhancement New feature or request scope: shared Both module related issues labels Nov 27, 2022
@Segfaultd
Copy link

Looks great! However, using direct properties instead of setter / getter could introduce a confusion between script-wrapped-entity and raw protocol / mod entity. Just my 2cts.

Finally v8pp 💯

@nevos08
Copy link

nevos08 commented Nov 27, 2022

Seems to be much easier :D

@eiksch
Copy link

eiksch commented Nov 27, 2022

I would rather love to see existing features being repaired/fixed/optimized instead of bringing non-impactful API changes for the big communities and the whole playerbase itself. I mean as a dev, I'm looking forward to it, but it will just takes us time to adapt/refactor to this aswell and the playerbase won't really feel the differences.

From the technical point of view all of the stuff listed above seems nice and cool to have, but not really necessary.

@duxeyo
Copy link

duxeyo commented Nov 27, 2022

I have to 1-up @Segfaultd. But other than that those are some great changes, especially the events system.

@Matspyder51
Copy link

Matspyder51 commented Nov 27, 2022

I would rather love to see existing features being repaired/fixed/optimized instead of bringing non-impactful API changes for the big communities and the whole playerbase itself. I mean as a dev, I'm looking forward to it, but it will cost us time to adapt/refactor to this aswell.

From the technical point of view all of the stuff listed above seems nice and cool to have, but not really necessary.

Like he said, the old API stay available and work, that give you the possibility to delay changes or do it one by one, it's also a way to optimize some features, like lazy properties, less C++ calls mean (in theory) a better execution time, and sooner it come, less code you gonna have to update

I agree too with @Segfaultd, it can create some confusion

@LeonMrBonnie
Copy link
Contributor Author

Looks great! However, using direct properties instead of setter / getter could introduce a confusion between script-wrapped-entity and raw protocol / mod entity. Just my 2cts.

Finally v8pp 💯

I assume you mean instead of having player.health we have player.getHealth() and player.setHealth(value) but I personally don't like this approach at all. I get the point that it differentiates between custom properties from scripts and built-in properties, but the additional work of writing it out seems to me to be more loss than what is gained from that.

I would rather love to see existing features being repaired/fixed/optimized instead of bringing non-impactful API changes for the big communities and the whole playerbase itself. I mean as a dev, I'm looking forward to it, but it will just takes us time to adapt/refactor to this aswell and the playerbase won't really feel the differences.

From the technical point of view all of the stuff listed above seems nice and cool to have, but not really necessary.

I get why you are saying this but you could say this forever, and then this would never happen. Its long overdue to fix the problems with the API. (Also, not only does this improve the API itself but will also massively speed up the maintaining of JS module itself, which is the thing that has drawn out most of my motivation ever since I started maintaining it)
At the end of the day, I will work on this mostly alone, and we still have other developers that will work on other alt:V things, so don't be scared that this would somehow massively slow down alt:V development.
(Also as a last point, I do this not only for the devs, but also for me, to preserve my own sanity working on this module for a longer time)

@LaszloR1
Copy link

Overall I like the changes, but I feel like moving to Factories introduces a bit too much abstraction at the cost of simplicity. Traditionally Factory methods are reserved for Factory classes, which this doesn't seem to be. Some purists might dislike that Vehicles both Vehicles and their own Factories. Although personally I'd dislike this change more if there were actual Factories involved. That said I understand that your hands are also tied due to v8pp, so there is that.

@LeonMrBonnie
Copy link
Contributor Author

Overall I like the changes, but I feel like moving to Factories introduces a bit too much abstraction at the cost of simplicity. Traditionally Factory methods are reserved for Factory classes, which this doesn't seem to be. Some purists might dislike that Vehicles both Vehicles and their own Factories. Although personally I'd dislike this change more if there were actual Factories involved. That said I understand that your hands are also tied due to v8pp, so there is that.

Yes unfortunately. v8pp brings a lot of value in maintainability, but the downside is that if you specify a constructor with it bound to a C++ class it always calls the actual C++ constructor of that class, which does not work with the way the SDK works.
Also as I said in the original post, it shows that the entity is owned by alt:V and not the script / the garbage collector itself.
An actual factory pattern doesn't really make much sense in JS due to it being so dynamic.

@LeonMrBonnie LeonMrBonnie pinned this issue Nov 27, 2022
@Yiin
Copy link

Yiin commented Nov 27, 2022

As an example, lets use the playerWeaponChange event here:

player.onWeaponChange(({ oldWeapon, newWeapon }) => {
    alt.log(`${this.name} switched their weapon from ${oldWeapon} to ${newWeapon}`);
});

The this context of the event handler passed here, will always be the entity itself.

It would be

player.onWeaponChange(function ({ oldWeapon, newWeapon }) {
   ...
});

because you can't bind context to arrow functions.

@abstractFlo
Copy link

As an example, lets use the playerWeaponChange event here:

player.onWeaponChange(({ oldWeapon, newWeapon }) => {
    alt.log(`${this.name} switched their weapon from ${oldWeapon} to ${newWeapon}`);
});

The this context of the event handler passed here, will always be the entity itself.

It would be

player.onWeaponChange(function ({ oldWeapon, newWeapon }) {
   ...
});

because you can't bind context to arrow functions.

you can. If the internal call is bind to the caller class. There is no issue with getting the context.

@LeonMrBonnie
Copy link
Contributor Author

It's just pseudocode to demonstrate what I mean.
Unsure if setting the this context in lambdas works when called via C++, I'll have to investigate that when I start actually working on this.

@xxshady
Copy link
Contributor

xxshady commented Nov 28, 2022

What do you think about adding enums directly into js module api, and not just typings (altv-types) so JS & TS users have full API access?
prebuild them using something like esbuild and implement it to c++ just like current JS bindings works?

+ I already have working solution for it in altv-esbuild

import { BlipSprite, KeyCode, RadioStation } from "altv-enums"

@leon20spr
Copy link

I think it's a good idea, also think that it improves a lot.

@LeonMrBonnie
Copy link
Contributor Author

What do you think about adding enums directly into js module api, and not just typings (altv-types) so JS & TS users have full API access? prebuild them using something like esbuild and implement it to c++ just like current JS bindings works?

import { BlipSprite, KeyCode, RadioStation } from "altv-enums"

We can do that, but only if its not directly in the alt namespace, because I don't want things that not everyone will use there. E.g. alt.Enums.KeyCode.ESC should be fine

@pixlcrashr
Copy link

pixlcrashr commented Nov 30, 2022

@LeonMrBonnie Good work so far. I really like the meta properties approach!

It's just pseudocode to demonstrate what I mean. Unsure if setting the this context in lambdas works when called via C++, I'll have to investigate that when I start actually working on this.

Before you start investigating, think about if setting the function/lambda (if it works) context is a good design principle and general idea.
Consider the following example:

Playground Link
(you might have to configure the TSConfig settings again (Target: ESNext, JSX: None, Module: CommonJS), thanks MS).

Now, inside my object's constructor (or other methods, too), I would not be able to access MyObject's properties over the this context. As you also can see, the function context is by default undefined.

What I am saying is that setting this to a non-default value creates confusion - at least for me, because I would expect lambdas to be able to access the "next object underlying" (e.g. MyObject).

Why not going (from my POV) the more logically way by adding the player to the context/args of the event?
Then, a developer directly see's: "Aha! I can access the oldWeapon, newWeapon and player properties on the context!" by just looking up the signature and not going into the documentation at all - self describing code/signature documentation is key imo.

To summarize, I think that setting this will create too much confusion, requires "too much" background knowledge to get started (as stated above - self documenting) and would also destroy the current expected behaviour of lambdas/functions inside other objects.

@xxshady
Copy link
Contributor

xxshady commented Nov 30, 2022

How deleting entity meta will work with new way?
delete entity.syncedMeta.key? And is it possible to do it like that?

@LeonMrBonnie
Copy link
Contributor Author

How deleting entity meta will work with new way? delete entity.syncedMeta.key? And is it possible to do it like that?

That will be possible, yes.

@xxshady
Copy link
Contributor

xxshady commented Dec 28, 2022

Hopefully there will be less memory leaks 🤞(I know this is too naive for C++, but still)

@LeonMrBonnie
Copy link
Contributor Author

Also a small addition so I don't forget in the future; The JS value to MValue serialization should be done (mostly) in JS, as the serialization in C++ is very slow. (That's why we have raw emits, but they are like half broken for some unknown reason)

@devxgaming

This comment was marked as off-topic.

@LeonMrBonnie

This comment was marked as off-topic.

@LeonMrBonnie
Copy link
Contributor Author

Since I forgot to post it here; https://github.com/altmp/altv-js-module-v2
The v2 module containing the aforementioned API rework is already open source for quite some time, and all other discussions regarding the API should be done in the issues of that repository.

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