Skip to content
This repository was archived by the owner on Mar 10, 2023. It is now read-only.

Make vm public - #9

Merged
poga merged 2 commits into
poga:masterfrom
voidxnull:expose_vm
Aug 29, 2018
Merged

poga merged 2 commits into
poga:masterfrom
voidxnull:expose_vm

Conversation

@voidxnull

Copy link
Copy Markdown
Contributor

Might be pretty useful. Alternatively, a getter method can be implemented instead.

@poga

poga commented Aug 27, 2018

Copy link
Copy Markdown
Owner

Do you have any use cases that requires direct access to the vm?

actix-lua configure its Lua vm with prelude.lua before loading user scripts. People can easily break it if they have direct access to the vm.

I do see the need of customizing the behavior of actix-lua. Maybe in the form of hooks?

@voidxnull

voidxnull commented Aug 28, 2018 •

Copy link
Copy Markdown
Contributor Author

I have a use case: to create lua bindings for external libraries (crypto, templates, etc.) Using messages to implement bindings seems unnecessarily clunky and non-reusable. Besides, how badly does one need to screw up to replace prefixed globals?

If you think that it's dangerous to make the vm completely public, there is another way to expose it: add a
new method LuaActorBuilder::with_vm which accepts a closure and pass the closure to LuaActor::new in LuaActorBuilder::build that can be used for initializing the vm before executing prelude.lua. That way all operations on the vm will be localized. I personally think this is unnecessary, but the decision is yours.

What do you think?

@poga

poga commented Aug 28, 2018 •

Copy link
Copy Markdown
Owner

My main concern is that by making it public, it encourages people to do whatever they want to the vm, including all the internal APIs. It makes all the internal Lua implementation public, make it hard to change the internal API without introduce breaking changes to the user.

I like the idea of LuaActorBuilder::with_vm👍! It clearly tells the user that "you should only do this when initializing a new actor".

Can you help me implement it? I can do it but you might have to wait.

@voidxnull

Copy link
Copy Markdown
Contributor Author

Ok, I'll do it.

@voidxnull

Copy link
Copy Markdown
Contributor Author

Done. I had to change the builder methods to take self by value to make it work.

@poga

poga commented Aug 28, 2018

Copy link
Copy Markdown
Owner

Can you add tests for it?

@voidxnull

Copy link
Copy Markdown
Contributor Author

Done.

@poga
poga merged commit 1e7d723 into poga:master Aug 29, 2018
@poga

poga commented Aug 29, 2018

Copy link
Copy Markdown
Owner

merged. Thanks!

@poga

poga commented Aug 29, 2018

Copy link
Copy Markdown
Owner

v0.3.1 released! 🎉

@voidxnull

Copy link
Copy Markdown
Contributor Author

Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants