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

JSRT APIs for Proxy #950

Closed
kunalspathak opened this issue May 9, 2016 · 12 comments
Closed

JSRT APIs for Proxy #950

kunalspathak opened this issue May 9, 2016 · 12 comments

Comments

@kunalspathak
Copy link
Contributor

Currently target and handler as internal fields of JavascriptProxy.cpp and there is no way to retrieve them natively. Recently node started taking dependency on these fields natively. This breaks node on chakracore as chakracore doesn't have these fields exposed via JSRT. Additionally as per ES6 spec, there is no way to check if an object is a proxy or not.

This issue is to track adding below JSRT APIs

  • GetTargetOfProxy
  • GetHandlerOfProxy
  • IsProxy
kunalspathak added a commit to kunalspathak/node-chakracore that referenced this issue May 9, 2016
Added APIs `GetTarget()`, `GetHandler()` for Proxy. Today, there is no
way to extract these fields through JSRT APIs, these APIs return
`undefined` for now. I have opened chakra-core/ChakraCore#950 to track
this.

Likewise, as per ES6 spec, there is no way to detect if an object is a Proxy
hence `IsProxy()` too needs an equivalent JSRT API.
kunalspathak added a commit to kunalspathak/node-chakracore that referenced this issue May 9, 2016
Added APIs `GetTarget()`, `GetHandler()` for Proxy. Today, there is no
way to extract these fields through JSRT APIs, these APIs return
`undefined` for now. I have opened chakra-core/ChakraCore#950 to track
this.

Likewise, as per ES6 spec, there is no way to detect if an object is a Proxy
hence `IsProxy()` too needs an equivalent JSRT API.
kunalspathak added a commit to kunalspathak/node-chakracore that referenced this issue May 9, 2016
Added APIs `GetTarget()`, `GetHandler()` for Proxy. Today, there is no
way to extract these fields through JSRT APIs, these APIs return
`undefined` for now. I have opened chakra-core/ChakraCore#950 to track
this.

Likewise, as per ES6 spec, there is no way to detect if an object is a Proxy
hence `IsProxy()` too needs an equivalent JSRT API.
kunalspathak added a commit to kunalspathak/node-chakracore that referenced this issue May 11, 2016
Added APIs `GetTarget()`, `GetHandler()` for Proxy. Today, there is no
way to extract these fields through JSRT APIs, these APIs return
`undefined` for now. I have opened chakra-core/ChakraCore#950 to track
this.

Likewise, as per ES6 spec, there is no way to detect if an object is a Proxy
hence `IsProxy()` too needs an equivalent JSRT API.

PR-URL: nodejs#69
Reviewed-By: Jianchun Xu <Jianchun.Xu@microsoft.com>
@dilijev
Copy link
Contributor

dilijev commented Feb 22, 2018

@digitalinfinity is this still needed?

@digitalinfinity
Copy link
Contributor

Yes, the v8 Proxy implementation in chakrashim is still just a stub

@rhuanjl
Copy link
Collaborator

rhuanjl commented Feb 22, 2018

Do you want a PR for these? Looks easy enough.

My only questions would be:
a) whether they should really be 3 separate APIs?
b) what sort of test should be there for this functionality - particularly if it's 3 separate functions adding them as callable methods in ch may start bloating ch's API a bit

@rhuanjl
Copy link
Collaborator

rhuanjl commented Feb 22, 2018

Maybe something like this:

JsGetProxyProperties(_In_ JsValueRef proxyObject, 
           _Out_ bool* isProxy,
           _Out_opt_ JsValueRef* Target, 
           _Out_opt_ JsValueRef* Handler);

@dilijev
Copy link
Contributor

dilijev commented Feb 23, 2018

Do you want a PR for these?

Sure! Let's hash out what's needed first and how we will test it. I'd imagine a NativeTests addition for this would be part of it.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Feb 27, 2018

Options for the API:

  1. One function:
JsGetProxyProperties(_In_ JsValueRef proxyObject, 
           _Out_ bool* isProxy,
           _Out_opt_ JsValueRef* Target, 
           _Out_opt_ JsValueRef* Handler);

Call it with a normal Proxy would output:
isProxy = true
And values for Target and Handler.

Call it with a revoked Proxy, would output:
isProxy = true
BUT nullptrs for Target and Handler

Call it with something that is not a proxy and you'd be given:
isProxy = false
And nullptrs for Target and Handler

  1. two functions:
    One for IsProxy - the other to get target and Handler.

  2. All 3 functions separately.

Options for testing:

  1. If done as one function could track it into ch and fire several tests at it from JS
  2. If done as multiple APIs should probably have a native test rather than implementing multiple new APIs in ch for one thing

Thoughts?

@dilijev
Copy link
Contributor

dilijev commented Feb 27, 2018

I like the elegance of one function. Let's explore that first. I think it's probably fairly likely if they want to know Target they will also want to know Handler, and need to guard accessing those on whether the object is a proxy at all.

There's value in keeping the APIs lean and this will be an experimental API at first so we could provide fewer APIs at first and go from there if there are perf concerns. If getting all the information has significantly more overhead and "all the info" is not the most common scenario in practice, then we can explore splitting up the API.

I can imagine this pattern being pretty common:

JsGetProxyProperties(obj, &isProxy, &target, &handler);
if (isProxy) { doSomethingWith(&target, &handler); }

@digitalinfinity what do you think? Also would you like this change targeted at master or release/1.9?

@jackhorton
Copy link
Contributor

@rhuanjl Any updates on this? If you don't plan on writing this, thats fine, but Node's new util.types.isProxy() requires this functionality.

@jackhorton
Copy link
Contributor

And, for that reason, I vote that any such change go into release/1.9 unless we plan on making 1.10 very soon.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Mar 8, 2018

@jackhorton I was waiting on further comment before writing anything as the ask didn't seem quite fixed yet.

I'll have time to write the code this weekend if we can be sure what's wanted before then.

@jackhorton
Copy link
Contributor

I agree with @dilijev about the design strategy. I don't think there will be a perf impact to get the target and handler, but also we could perhaps pass null for those if we don't want that information and are concerned about doing extra work.

chakrabot pushed a commit that referenced this issue Mar 14, 2018
…fixes #950

Merge pull request #4806 from rhuanjl:JsGetProxyProperties

Responding to issue #950

This PR:
1. Adds a JavascriptProxy::IsRevoked method to the Javascript proxy class (necessary for the below)
2. Adds a JsGetProxyProperties API to Jsrt which can:
    a) check if an object is a proxy -> set a provided bool to true/false
    b) if it is a proxy check if it's revoked
    c) if it is a revoked proxy set provided target and handler references to nullptr
    d) if it is a proxy that is not revoked provide references to it's target and handler
3. Tracks the same API through to WScriptJsrt in ch
4. Adds a test that uses the ch implementation

(Targeting 1.9 as this will assist with an issue in node-chakracore nodejs/node-chakracore#488 )

**CC:** @jackhorton @kfarnung @dilijev
@jdalton
Copy link

jdalton commented Apr 25, 2018

Just a heads up chakra-node has shipped v10 so now beyond util.inspect() with showProxy not working there is also util.types.isProxy() which doesn't work either.

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

6 participants