-
-
Notifications
You must be signed in to change notification settings - Fork 13
Add support for "external" values #107
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
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 at least on cgo.Handle that doesn't get released.
// _internal field_. | ||
func getInstance(info *v8.FunctionCallbackInfo) *Calculator { | ||
var internalField *v8.Value = info.This().GetInternalField(0) | ||
var handle cgo.Handle = internalField.ExternalHandle() |
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.
This handle needs to be deleted!
Specifically as this serves as a public example - it should be clear that handles need releasing.
Many places a value is created from a V8 ptr as &Value{ptr: val} - i.e., the ctx isn't set.
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 suggest moving this into example/
(and skipping the example_
prefix). The root is getting pretty large.
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 agree on the "pretty large" - but does it work the same? Is the example embedded in the documentation the same way?
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.
Oh, right. No, that won't work.
This example feels large enough that it might make sense to keep it separate anyway. If it could be made into 30 lines, I think having it next to the function is reasonable (and then we wouldn't have one file per example case).
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.
The "one file per example" is a special case where the file is included in its entirety which I chose for this as the Calculator
is a crucial part of the example - it's about how to create a JS wrapper object on top of a native type - a crucial part for my own use case.
Of course, a different example could be created using an internal go type, e.g., create a stack, where the value is a *[]v8.Value
or something - then the example function could stand on it's own. The calculator was just the use case I came up with when writing the example.
The name of the example function dictates where in the docs it's included - right now it's associated with NewValueExternalHandle
- but maybe it shouldn't be assiciated with this function specifically, as the use case requires the use of Set
/GetInternalValue
and InternalValueCount
- and how to create the values in the constructor.
E.g., this is from my own godoc server running

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.
Btw, looking at the example, I notice something that should probably be handled differently
var internalField *v8.Value = info.This().GetInternalField(0)
var handle cgo.Handle = internalField.ExternalHandle()
calculator, ok := handle.Value().(*Calculator)
if !ok {
panic("Not a calculator")
}
If the field can't be looked up, it should return a TypeError
(I guess that's the appropriate return value). An example JS code that would be affected
const notACalculator = { __proto__: Calculator.prototype }
notACalculator.push(1)
As it's setting the prototype, it would call the go callback, but as this wasn't created through the constructor, it wouldn't have the internal field. I experienced a similar bug in my own code.
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 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.
The name of the example function dictates where in the docs it's included - right now it's associated with NewValueExternalHandle - but maybe it shouldn't be assiciated with this function specifically, as the use case requires the use of Set/GetInternalValue and InternalValueCount - and how to create the values in the constructor.
Yeah, I think the example is too long for being tied to a single function. It feels unfocused, even if the example is good. Perhaps flipping it so the relevant function invocation is first would help shake that feeling.
I think I have a good alternate example to use - so I hope to get it rewritten this week. Instead of the That would allow creating an example that speaks for itself, without any external context. The purpose of the example is to show how to create a JS wrapper object on top of a native Go object, how JS code can call into the object, and how you can retrieve the Go object from a JS object. Then I'll fix missing handle cleanup and prop handling of mismatched prototype. |
"External" values in V8 are designed to contain pointers, when a V8 ObjectTemplate acts as a proxy/wrapper for an internal object.
I exposed two different ways of accessing them, as
unsafe.Pointer
andcgo.Handle
values; but with a strong preference to cgo Handles.The ability to store Handles makes function callback handling significantly easier to deal with, which I am taking advantage of in the prototype to implement named/indexed property handlers.
It's actually been quite a long time since I wrote this - can't remember the details, but I can see I made a larger example demonstrating usage.
But I do see that I forgot to cleanup the handle in the example. I need to do that.