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

Support creating a global ObjectTemplate into a context #64

Merged
merged 9 commits into from
Jan 25, 2021

Conversation

rogchap
Copy link
Owner

@rogchap rogchap commented Jan 17, 2021

Provides creating a global object that can be initiated within each Context eg:

iso, _ := v8go.NewIsolate()
obj, _ := v8go.NewObjectTemplate(iso)
obj.Set("version", "v1.0.0", v8go.ReadOnly)
ctx, _ := v8go.NewContext(iso, obj)
val, _ := ctx.RunScript("version", "main.js")
fmt.Println(val)
// Output:
// v1.0.0

The same ObjectTemplate can be used in many Contexts as long as they are part of the same Isolate (VM).

This PR also adds the ability to create primitive values that can be properties on the global object.

This is the prerequisite to FunctionTemplates for the global context.

As a side-effect of this PR. The NewContext() API has changed to optionally accept a Isolate and a Global Template, this makes for a nice API as you see above, but also allows for this:

ctx, _ := v8go.NewContext()    // previously you would do v8go.NewContext(nil)

This is fully backwards compatible.

I've not updated the README.md to not confuse readers, until this is released into a new version.

@rogchap rogchap mentioned this pull request Jan 17, 2021
@rogchap rogchap marked this pull request as ready for review January 19, 2021 08:23
@rogchap rogchap requested a review from tmc January 19, 2021 08:25
Copy link
Collaborator

@tmc tmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great -- mostly minor comments.

object_template.go Show resolved Hide resolved
object_template.go Outdated Show resolved Hide resolved
object_template.go Outdated Show resolved Hide resolved
object_template.go Outdated Show resolved Hide resolved
object_template.go Outdated Show resolved Hide resolved
object_template.go Show resolved Hide resolved
value.go Show resolved Hide resolved
context_test.go Outdated Show resolved Hide resolved
@tmc
Copy link
Collaborator

tmc commented Jan 22, 2021

mentioned in the diff inline but in the PR description as well, the Set call's attribute just being passed as "0" is a bit blah.

perhaps we should consider a different api to set the property attributes so that could be omitted in the normal case?

@rogchap
Copy link
Owner Author

rogchap commented Jan 22, 2021

Set call's attribute just being passed as "0" is a bit blah.
perhaps we should consider a different api to set the property attributes so that could be omitted in the normal case?

That's me just being lazy. I'll change the examples to use the enum/const.
I'm open to suggestions on the API, however this is the API that is provided by the C++ API, and I want to keep as true to that as possible.

For ObjectTemplate's it's more likely that you want to set ReadOnly rather than None anyway.

Ps. I'm not a huge fan of APIs that have prepositions (e.g. "For", "With", "At", "To") if they can be avoided; adding an argument to a method makes more sense unless the method has a distinct verb. The challenge is that Go does not have overloaded methods, but C++ does, so there will be some compromise needed here (from me).

@tmc
Copy link
Collaborator

tmc commented Jan 22, 2021

That's fair, I think attempting to match the c++ makes sense (and I know it's stated goal).

It's just too bad that something as succinct as x.Set("foo", "bar") isn't possible with this PR due to the PropertyAttribute parameter. It seems as though it's going to be pretty rare for anything but ReadOnly, at least to start.

I'm curious about the use case in mind for exposing the properties here and if we can grow the api surface area at a later time.

@rogchap
Copy link
Owner Author

rogchap commented Jan 22, 2021

It's just too bad that something as succinct as x.Set("foo", "bar") isn't possible

The goals do state to match the C++ API, but still as idiomatic Go as possible (our get out of jail free card 😉 ).
What we could do is change this API to a variadic function, similar to what we are doing for Context.

This would make the property attribute optional and default to None.

Ps. the PropertyAttribute is also used for normal Objects (not template ones); so would have to do the same there too.

What do you think @tmc ?

@rogchap
Copy link
Owner Author

rogchap commented Jan 23, 2021

So thinking about this a bit more... rather than a varadic function with "SetOptions"... what about just doing a varadic function of PropertyAttributes, because they are flags that can be added together?

This is better explained via examples:

Current API

func (o *ObjectTemplate) Set(name string, val interface{}, attributes PropertyAttribute) error 

// the caller would do:
obj.Set("foo", "bar", v8go.ReadOnly)
// or for multiple:
obj.Set("foo", "bar, v8go.ReadOnly | v8go.DontEnum)

Proposed API (v2)

func (o *ObjectTemplate) Set(name string, val interface{}, attributes ...PropertyAttribute) error 

// previous examples would still be valid
obj.Set("foo", "bar", v8go.ReadOnly)
obj.Set("foo", "bar, v8go.ReadOnly | v8go.DontEnum)

// but would also allow for this
obj.Set("foo", "bar")
obj.Set("foo", "bar, v8go.ReadOnly, v8go.DontEnum)

In the last example the slice of attributes would be converted to OR ie.

var attrs PropertyAttribute
for _, a := range attributes {
    attrs |= a
}

// ie: 

obj.Set("foo", "bar, v8go.ReadOnly, v8go.DontEnum)
// is equal to 
obj.Set("foo", "bar, v8go.ReadOnly | v8go.DontEnum)

@rogchap rogchap requested a review from tmc January 23, 2021 01:57
Copy link
Collaborator

@tmc tmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rogchap rogchap merged commit b35f871 into master Jan 25, 2021
@rogchap rogchap deleted the object_template branch January 25, 2021 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants