-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat(scripting/v8): expose msgpack codec globally for custom type sup… #2931
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
base: master
Are you sure you want to change the base?
Conversation
…port - Added `global.msgpack_codec` to provide access to the msgpack codec. - Enables developers to easily add custom serializers/deserializers. - Maintains backward compatibility with existing msgpack_pack/unpack functions.
|
Here is a link to my documentation pull request: citizenfx/fivem-docs#503 |
|
Personally I'd like to see a modern and more efficient msgpack implemented before we get access to the codec. The current one hasn't been touched for 8 years and is lacking a lot of JS features. https://github.com/kawanet/msgpack-lite @AvarianKnight might also have some thoughts on this. |
There's a lot better encoders/decoders for msgpack now and it would make a lot of sense to swap to them (see msgpackr for one that's really fast, and blows out all current implementations) |
Thank you for the suggestion! (I don't know if this message is directed to the right place.) Here’s what I’ve managed so far and the issues I’ve encountered:
Note: I have not made any test with C#. These challenges would require additional development to resolve and could impact other parts of the project. I’d love to hear your thoughts or suggestions on addressing these issues. Additionally, if you'd like, I can share the code I’ve worked on so far to help provide more context. Please let me know where and how I should share it. |
|
Hello again 👋 Now that the migration to The goal here was not to replace the codec, but to expose the existing one globally, so developers can register custom serializers/deserializers using This would provide more flexibility for advanced scripting scenarios while maintaining full backward compatibility. Since the codec ( I'm happy to rebase and update the PR if needed. @Nobelium-cfx – would love to hear your thoughts on this. 🙂 |
I dont really know much about script runtimes and I understand the general argument for flexibility. But, to be honest, I have a hard time coming up with a scenario where this would be actually useful. Can you provide a particular example of a feature that can't be implemented without this (or is significantly harder to implement)? Another concern is: do we only execute |
One practical use-case is sending a large array of objects that all share the same properties in an event. Right now the only way to control packing at that level is to use TriggerServerEventInternal on the sender and global.addRawEventListener on the receiver, to bypass msgpack-lite serialisation. It's still possible, but involve using 'internal' functions |
Thanks for the feedback, @Nobelium-cfx 🙏 Let me try to clarify a few points that might help address your concerns: ✅ No New Vulnerability IntroducedThis PR does not introduce any new capability that wasn’t already available internally. The This change simply exposes the codec via a global Developers can then register custom types using There is no way for this to break out of the sandbox or gain additional permissions. It’s just surfacing an internal object for extensibility. 🔐 Still Fully SandboxedAll usage remains:
So from a security standpoint, this doesn’t open any new attack surface—it simply enables advanced use cases for developers already operating within safe boundaries. 🎯 Real-World Use CaseImagine a developer needs to serialize a custom class like const codec = msgpack.createCodec({
preset: true,
binarraybuffer: true,
uint8array: true
});
class Money {
constructor(public amount: number, public currency: string) {}
}
codec.addExtPacker(0x42, Money, (money) => [money.amount, money.currency]);
codec.addExtUnpacker(0x42, ([amount, currency]) => new Money(amount, currency));
addEventListener('money', (rawMoney) => {
const [amount, currency] = msgpack.decode(rawMoney, {codec});
const money = new Money(amount, currency);
console.log(money, money instanceof Money); // Output { amount : 10, currency: '$' }, true
});
const money = new Money(10, '$');
TriggerEvent('money', msgpack.encode(money, {codec}));This works, but adds overhead and repeated boilerplate. By exposing class Money {
constructor(public amount: number, public currency: string) {}
}
msgpack_codec.addExtPacker(0x42, Money, (money) => [money.amount, money.currency]);
msgpack_codec.addExtUnpacker(0x42, ([amount, currency]) => new Money(amount, currency));
addEventListener('money', (money) => {
// No more msgpack.decode call here
console.log(money, money instanceof Money); // Output { amount : 10, currency: '$' }, true
});
TriggerEvent('money', new Money(10, '$')); // No more msgpack.encode call hereThis not only reduces complexity and error-prone conversions, but also enables cleaner architecture when sharing complex types across JS ↔ Lua boundaries. I'm totally open to suggestions—whether it’s renaming the global or limiting visibility to certain scopes—to make this more comfortable for review. But I do think this provides tangible value to developers without compromising safety. In short, this change just gives devs access to a tool they already rely on indirectly—now with full control and clarity. Looking forward to your thoughts! 🙂 |
|
What is with people and using AI for GitHub issues and PRs? It's painfully obvious that it's AI, it does nothing but take away from what you are trying to say |
He doesn't know a lot about msgpack so i explain it to him but i'm not english so i use IA to make my comment better. |
if he wanted an AI response he couldve done it himself, theres no way of knowing if this is your words or some randomly generated non sense |
|
While its AI generated, its all (mostly) correct. You wouldn't be able to decode the type without adding to the codec, since you don't have access to the codec. There isn't any issues relating to sandboxing, since extending the msgpack codec would be per-ScRT, Lua already has the capability to do this.
Sending Vectors out of the JS runtime wont encode properly to a Lua version, this makes cross compat between the two annoying (at least in regards to sending vectors, though someone figured out an extremely hacky work around to this to get the codec, but I don't remember what it was) This also isn't anything new, Lua already exposes its codec. |
In my case it's to make instance replicated in both client/server in JS env using this codec So i can create Ped instance server side and get it back in client side for example. |
Nobelium-cfx
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thank you all! I think it looks good. Shouldn't make any harm indeed.
PS: I think its ok to use AI if it helps you. But I do feel that #2931 (comment) wasn't the most useful comment :). It has more water than meaningful context.
Goal of this PR
Expose the msgpack codec globally to enable developers to easily add custom serializers and deserializers, enhancing flexibility for handling custom types in FiveM scripts.
How is this PR achieving the goal
This PR adds a new global variable, global.msgpack_codec, which provides direct access to the existing msgpack codec. Developers can use this to register custom types via addExtPacker and addExtUnpacker while maintaining backward compatibility with the current global.msgpack_pack and global.msgpack_unpack functions.
This PR applies to the following area(s)
Successfully tested on
Game builds:: 3258
Platforms: Windows
Checklist
Fixes issues
No specific issues reported for this feature addition.