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

Can add context support? #243

Closed
mei-rune opened this issue Jan 13, 2021 · 6 comments
Closed

Can add context support? #243

mei-rune opened this issue Jan 13, 2021 · 6 comments

Comments

@mei-rune
Copy link

add 3 functions

func (r *Runtime) RunProgramWithContext(ctx context.Context, p *Program) (result Value, err error)
func (r *Runtime) RunScriptWithContext(ctx context.Context, name, src string) (Value, error)
func (r *Runtime) RunStringWithContext(ctx context.Context, str string) (Value, error)

add Context field into FunctionCall and ConstructorCall.

usage:

vm := New()
vm.Set("test", func(call goja.FunctionCall) goja.Value {
  ctx := call.Context
  ........
})
......

vm. RunStringWithContext(ctx, `test('aa')`)
@dop251
Copy link
Owner

dop251 commented Jan 13, 2021

See #198

@mei-rune
Copy link
Author

My Needs is Pass a Argument from RunXXXWithContext to my go function.

vm := New()
vm.Set("test", func(call goja.FunctionCall) goja.Value {
  ctx := call.Context
  x := ctx.Value("xxxx").(MyObject)
  ........
})
......

vm. RunStringWithContext(context.WithValue(ctx, "xxxx", myObject), `test('aa')`)

yes, context is error, but it is already exists in the golang, and gls isnot exists in the golang.
There is now a way to work around, but it is not convenient, and inefficient:

vm := New()
vm.Set("test", func(call goja.FunctionCall) goja.Value {
   myObject := convertToMyObject(call.Argument(0))
  ........
})
......
vm.Set("xxx", myObject)
vm. RunString(`test(xxx, 'aa')`)
vm.Set("xxx", goja.Undefined())

@mstoykov
Copy link
Contributor

HI @runner-mei,

At k6 we use this function to basically do a struct->map[string]interface{} transformation such that for each method of the struct that has context.Context as the first argument it will be given the context provided in the Bind call, but all other arguments remain the same.
(It also does other things that are less relevant to the discussion here).

I agree that having this build inside goja would've saved us doing that (although it is from before my time, and we are kind of afraid to touch it ... as it's a lot of black magic). On the other hand, this hack isn't as ugly as it looks, and apart from having to know to call it when this is required and the fact that you can't use the resulting value as the type it originated as (probably butchering the explanation somewhat) it is ... perfectly serviceable and is used throughout k6. I would expect that other people have also done similar hacks or that this is a very rare use case?

Now that I am looking at the code I am more and more inclined to believe that if we don't export map[string]interface{} but a Proxy (like) object this will probably work better and there will be a way for the common.Bind to even get the correct object out of the proxy and put it back as the argument for the call. So the issue from our community forum is solvable even now :).

Will it be easier for me in this case if goja, just supported it - definitely, although I don't know if it won't have other problems.

If I was writing a JS VM in golang, today I am definitely adding context support, but retroactively adding it, might be much harder and likely will break existing code so 🤷

Another alternative that I am probably going to explore in k6 is to drop the whole binding of stuff and just getting goja.FunctionCall in all cases where we care about the context.Context and having the context.Context as a global in the goja.Runtime that we get from the goja.FunctionaCall Unfortunately as I wrote this I went and checked and it seems like there is no way to get the goja.Runtime :(

@dop251
Copy link
Owner

dop251 commented Jan 13, 2021

I wouldn't want to add Runtime to FunctionCall simply because in most cases it's not needed, however, it should be possible to have a special treatment for func(FunctionCall, *Runtime) Value in Runtime.ToValue() which would make a simple wrapper around it. If it helps.

@mstoykov
Copy link
Contributor

I wouldn't want to add Runtime to FunctionCall simply because in most cases it's not needed, however, it should be possible to have a special treatment for func(FunctionCall, *Runtime) Value in Runtime.ToValue() which would make a simple wrapper around it. If it helps.

If this is so my last alternative will work ... I don't think it is required for the k6 team, at least not currently.

I don't find either of the two shortcomings big enough to touch the code I linked ... as it is a lot of black magic. So removing or completely rewriting it .. is an unlikely occurrence. I do intend on dropping support for a pointer to a context ... which I don't know why exists and maybe at that time I will try to figure out if there are other things I would be willing to try changing. The problem is that at least for our project - having another "easier" way of doing stuff isn't really interesting as we already have it working :D

This doesn't mean that I don't think that something like

func(f FunctionCall, r *Runtime)

or even better

func(parameter1 int, r *Runtime)

won't be useful. I just prefer if you work on adding support for more ES6 features such as generators/Promises/let/const then adding random wrapping logic that at least for k6 we have already figured out, and rewriting to an alternative variant will likely ... not improve things for us.

Others obviously might be in a different boat, and I can definitely say that some of the cases where context is being used in k6 are just so we get the Runtime and do something with it, not to get any of the other things the context has or check if it's "Done".

@mei-rune
Copy link
Author

mei-rune commented Jan 16, 2021

I don't like "func(f FunctionCall, r *Runtime)", I can achieve this by myself。

https://github.com/runner-mei/gojs/blob/main/main.go, this implementation only a few small flaws:

  1. debug info "function name" will missing.
  2. source code is duplicated.

If add a ToValue hook to goja, it will perfect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants