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

Function Call should support recv argument #159

Merged
merged 7 commits into from
Sep 9, 2021

Conversation

genevieve
Copy link
Collaborator

(cherry picked from commit 991ab09)

Fixes #156

The v8go library is implicitly passing undefined as the receiver when executing a function Call. To make this configurable, as shown in this PR, it would be a breaking change to the v8go api for the function object. This change ensures the explicit passing of a receiver to a function and avoid additional confusion around the fact that functions obtained from an object are not bound to the object they are obtained from. If we want to avoid the breaking change, we could instead expose something on the v8go object type called MethodCall which would ensure the proper execution a function/method with the object as it's receiver and avoid code like:

respVal, _ := ctx.Get("response")
respObj, _ := respVal.AsObject()
textVal, _ := respObj.Get("text")
textFn, _ := textVal.AsFunction()
textProm, _ := textFn.Call(respObj)
...

And instead may look like:

respVal, _ := ctx.Get("response")
respObj, _ := respVal.AsObject()
textProm, _ := respObj.MethodCall("text")
...

The choice is essentially - a breaking change that aligns the function call with the v8 api or deviating a little from the v8 api by exposing a new method on objects.

@codecov
Copy link

codecov bot commented Sep 1, 2021

Codecov Report

Merging #159 (aeeeab1) into master (f985b21) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
- Coverage   97.50%   97.46%   -0.05%     
==========================================
  Files          12       12              
  Lines         481      473       -8     
==========================================
- Hits          469      461       -8     
  Misses          8        8              
  Partials        4        4              
Impacted Files Coverage Δ
function.go 100.00% <100.00%> (ø)
object.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f985b21...aeeeab1. Read the comment docs.

@genevieve genevieve marked this pull request as ready for review September 1, 2021 18:29
@dylanahsmith
Copy link
Collaborator

If we want to avoid the breaking change, we could instead expose something on the v8go object type called MethodCall which would ensure the proper execution a function/method with the object as it's receiver

As I mentioned in #156 (comment), I think that would be a useful convenience method even with the breaking change. It would also reduce the need for using Function.Call directly.

Making the receiver explicit in Function.Call is independently useful in making Function.Call more generally useful when passing an explicit receiver is necessary, to avoiding the confusion that comes from implicitly passing undefined and to keep the API more consistent with the v8 API.

@genevieve
Copy link
Collaborator Author

Ok I will leave this PR here and we can merge it if passes review and we plan to ship a release of v8go with the breaking change. I can make a separate PR for the new object.methodcall suggested.

function.go Outdated Show resolved Hide resolved
@dylanahsmith dylanahsmith requested a review from rogchap September 8, 2021 15:36
Copy link
Owner

@rogchap rogchap left a comment

Choose a reason for hiding this comment

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

One small comment; otherwise looks good.

I'm ok with the breaking change as we are on 0.x and we have already made some breaking changes with using panic rather than returning null errors.
Would be good to document some of these breaking changes in the Readme.

function.go Outdated Show resolved Hide resolved
@dylanahsmith
Copy link
Collaborator

Would be good to document some of these breaking changes in the Readme.

They are being documented in the CHANGELOG.md and when a release is made, we could copy that text to the release description. Those seem like the places where someone would go to find information on changes relevant to an upgrade.

Probably the main point of confusion with the README is that someone encountering the project for the first time will see the README for the master branch rather than for the latest release, which is a common problem with reading it on github. At least it will show the right thing when viewing the documentation (https://pkg.go.dev/rogchap.com/v8go#section-readme), so perhaps we could refer them for the latest release documentation if there are unreleased breaking changes that affect the README.

@dylanahsmith dylanahsmith merged commit 826c7ca into rogchap:master Sep 9, 2021
@dylanahsmith dylanahsmith deleted the gl-fn-recv branch September 9, 2021 20:21
@dylanahsmith
Copy link
Collaborator

Alternatively, perhaps it would be a good time to make a new release, which would also avoid the confusion from unreleased breaking changes.

@rogchap
Copy link
Owner

rogchap commented Sep 12, 2021

I think this is a good idea; a new release is overdue.

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.

Calling an instance method that uses this fails with undefined
3 participants