Closed LeonMrBonnie closed 1 year ago
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 💯
Seems to be much easier :D
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 have to 1-up @Segfaultd. But other than that those are some great changes, especially the events system.
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
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)
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.
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.
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.
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.
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.
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"
I think it's a good idea, also think that it improves a lot.
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"
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
@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.
How deleting entity meta will work with new way?
delete entity.syncedMeta.key
? And is it possible to do it like that?
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.
Hopefully there will be less memory leaks 🤞(I know this is too naive for C++, but still)
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)
Can you please also added in new API, access to all webview listeners, for example:
alt.getEventListeners() // is exists in current API
alt.getWebViewEventListeners() // all active webview listeners.
maybe it can be helpful to catch cheaters interface menu if is using alt.WebView
!
Can you please also added in new API, access to all webview listeners, for example:
maybe it can be helpful to catch cheaters interface menu if is using
alt.WebView
!
webview.getEventsListeners()
also exists already, please don't use this thread as a way to report bugs / suggest general new APIs.
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.
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:
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)
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: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
andalt-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 thePlayer
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 ownPlayer
class. Instead of doing this, we will have aSharedPlayer
class (that is not accessible from JS code) which is in the shared side of the module, and the individualPlayer
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.