-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
Function Templates with callback functions in Go #68
Conversation
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.
There's a lot going on here and I can't say I grok every line but I'm going to approve as the tests look good and I don't see glaring errors!
} | ||
|
||
// NewFunctionTemplate creates a FunctionTemplate for a given callback. | ||
func NewFunctionTemplate(iso *Isolate, callback FunctionCallback) (*FunctionTemplate, error) { |
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.
how does vm.TerminateExecution
interact with callback of v8go.NewFunctionTemplate
?
would it be possible to include context.Context
as first argument of callback?
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.
Once the VM is terminated, you will no longer be able to fire the callback. These callbacks are connected to the isolate, so once the Isolate is GC'd so will be the callback functions (unless attached to some other struct etc)
You can get the current context via info.Context()
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.
context.Context
will get confusing with v8go.Context
(as I just did)
The V8 API does not have such a concept as the Go Context; if we introduce the Go Context, this would have to exist throughout the API, which would likely be a breaking change.
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 had a look at your code (https://github.com/choonkeat/try-v8go/blob/main/main.go)
I see what you are trying to do... you want to be able to check if the Go Context is Done
within the callback?
Maybe we could attach a context in a way that is non-breaking ie. something like info.GoContext()
??
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.
@choonkeat Would you be able to submit this Issue as a feature request? It's maybe something we could look to supporting.
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.
context.Context will get confusing with v8go.Context (as I just did)
ya :-P
Would you be able to submit this Issue as a feature request?
it was more of a comment, since idiomatic Go callback function usually comes with context.Context
. But if it doesn't fit v8, I don't mind passing my own context.Context
via closure
i'm still trying to wrap my head around how things "should" work. e.g. implementing fetch
i'd need to return a Promise i think and thus will be tracking #61 closely
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.
Yes, to implement true fetch
you need to return the Promise; A poor man's version is here: https://github.com/rogchap/v8go/blob/master/function_template_test.go#L52
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.
FYI. this is better supported now that we have #76
Basic
FunctionTemplate
support, which allows global functions that callback to Go functions:Due to the complexities of Go -> C -> Go there are two registries:
To make sure that created Contexts can be GC'd we have a register/deregister surrounding script execution; other wise a
map[int]*Context
would never GC