Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

util: util.types.isProxy does not work in Node-ChakraCore #488

Closed
jackhorton opened this issue Mar 7, 2018 · 10 comments
Closed

util: util.types.isProxy does not work in Node-ChakraCore #488

jackhorton opened this issue Mar 7, 2018 · 10 comments
Labels
compatibility modified test

Comments

@jackhorton
Copy link
Contributor

Causes test/parallel/test-util-types to fail.
@jackhorton jackhorton added this to the Node 10 Release milestone Mar 7, 2018
@kfarnung
Copy link
Contributor

kfarnung commented Mar 8, 2018

Refs: chakra-core/ChakraCore#950

@jackhorton jackhorton removed this from the Node 10 Release milestone Mar 8, 2018
@jackhorton
Copy link
Contributor Author

See 1e9ba64#diff-1980aca1cba9a1f0b7278ab1db2c064cR54

@rhuanjl rhuanjl mentioned this issue Mar 10, 2018
Merged
chakrabot pushed a commit to chakra-core/ChakraCore that referenced this issue Mar 13, 2018
@boingoing
[MERGE #4806 @rhuanjl] Implement JsGetProxyProperties API fixes #950
6861888 
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
chakrabot pushed a commit to chakra-core/ChakraCore that referenced this issue Mar 14, 2018
@boingoing
[1.9>master] [MERGE #4806 @rhuanjl] Implement JsGetProxyProperties API …
707ae66 
…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
chakrabot pushed a commit that referenced this issue Mar 14, 2018
@boingoing @chakrabot
deps: update ChakraCore to chakra-core/ChakraCore@6861888a15
f7e7601 
[MERGE #4806 @rhuanjl] Implement JsGetProxyProperties API 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 #488 )

**CC:** @jackhorton @kfarnung @dilijev

Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>
chakrabot pushed a commit that referenced this issue Mar 14, 2018
@boingoing @chakrabot
deps: update ChakraCore to chakra-core/ChakraCore@707ae66637
1905e61 
[1.9>master] [MERGE #4806 @rhuanjl] Implement JsGetProxyProperties API 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 #488 )

**CC:** @jackhorton @kfarnung @dilijev

Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>
kfarnung pushed a commit that referenced this issue Mar 14, 2018
@boingoing @kfarnung
deps: update ChakraCore to chakra-core/ChakraCore@6861888a15
1a28e99 
[MERGE #4806 @rhuanjl] Implement JsGetProxyProperties API 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 #488 )

**CC:** @jackhorton @kfarnung @dilijev

Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>
@jdalton jdalton mentioned this issue Apr 25, 2018
Closed
@rhuanjl
Copy link
Contributor

rhuanjl commented Apr 25, 2018

Is there a reason not to support util.types.isProxy? The JsGetProxyProperties api I added to ChakraCore should enable supporting it. Unsure what the policy for contributing to node-chakracore - but I could try and make a PR to implement this if wanted?

@kfarnung
Copy link
Contributor

No reason not to support it, we just hadn't gotten the chance to implement it yet. The contribution policy is the same as upstream node, feel free to open a PR.

@rhuanjl
Copy link
Contributor

rhuanjl commented Apr 26, 2018

@kfarnung Thanks - ok I'll give it a try and I'll see if I can do #523 at the same time.

One question though, is there an existing mechanism for exposing a native function to chakra_shim.js without exposing it globally? (I can't currently find such in the source tree though I may just not be looking closely enough)

@jackhorton
Copy link
Contributor Author

I don't think there is. With that being said, isProxy doesn't have to be in chakra_shim.js, it can be implemented in native (I think most of the other basic types are done in native, and only complex types like MapIterator and SetIterator are in chakra_shim.js). In general, the JS file is a grab bag of "we don't have a native API to do this, but we know how to do it in JS, so do it in JS". Now that we have a native API, the JS version shouldn't be necessary. There will be a relevant change in v8value.cc to remove IS_TYPE_FUNCTION(IsProxy, isProxy) and add a real native implementation (bool Value::IsProxy() const { })

@rhuanjl
Copy link
Contributor

rhuanjl commented Apr 26, 2018 •
edited
Loading

Ok so I've made working fixes for this and util.inspect's showProxy flag. But trying to run make test before opening a PR I get multiple errors that are nothing to do with proxies. Are there tests that you expect to fail at the moment?

e.g.s

  1. test/parallel/test-console-no-swallow-stack-overflow.js -> crashed
  2. test/parallel/test-console-sync-write-error.js -> timeout
  3. test/parallel/test-util-types.js -> has an ESModule test which fails as CC doesn't support ESModules
  4. test/parallel/test-http2-respond-file-fd-invalid.js -> fails by calling mustNotCall()
  5. test/message/stack_overflow.js -> crashed

I can put an if (!common.isChakraEngine) around the ESModule test in test-util-types.js but unsure on the other ones.

@kfarnung
Copy link
Contributor

make test already ignores some tests (take a look at the *.status files) and should come back clean, go ahead and open a PR and we can figure out what’s going on with the tests.

@jackhorton
Copy link
Contributor Author

I am also surprised to hear that make test isn't working for you, since we do official node CI runs as well as internal CI runs on Linux multiple times a day. I haven't personally ran node-chakracore tests on Linux recently, but does this have something to do with how on windows, we usually specify vcbuild.bat test-ci ignore-flaky rather than just vcbuild.bat test?

@MSLaguana
Copy link
Contributor

On Linux, we use FLAKY_TESTS=dontcare make test-ci and don't expect any unmarked failures (unless there are machine configuration issues). I just double checked our latest CI and the only failures there are artifacts of the vm that we run it in, and none of them are ones you mention above.

@rhuanjl rhuanjl mentioned this issue Apr 26, 2018
Closed
2 tasks
rhuanjl added a commit to rhuanjl/node-chakracore that referenced this issue May 1, 2018
@rhuanjl
chakrashim: implement proxy methods in chakrashim
cbf6346 
Implement value::IsProxy(), Proxy::GetTarget() and
Proxy::GetHandler() in chakrashim. This  enables
util.types.isProxy() and util.inspect() with the
showProxy flag as true to function correctly in
node-chakracore.

Also enable related tests for node-chakracore:
parallel/test-util-types.js
parallel/test-util-inspect.js

fixes: nodejs#488
fixes: nodejs#523
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this issue May 9, 2018
@rhuanjl @kfarnung
chakrashim: implement proxy methods in chakrashim
3ffc843 
Implement value::IsProxy(), Proxy::GetTarget() and Proxy::GetHandler()
in chakrashim. This enables util.types.isProxy() and util.inspect()
with the showProxy flag as true to function correctly in
node-chakracore.

Also enable related tests for node-chakracore:
* parallel/test-util-types.js
* parallel/test-util-inspect.js

Fixes: nodejs#488
Fixes: nodejs#523
PR-URL: nodejs#525
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Jack Horton <Jack.Horton@microsoft.com>
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this issue May 10, 2018
@rhuanjl @kfarnung
chakrashim: implement proxy methods in chakrashim
e04616c 
Implement value::IsProxy(), Proxy::GetTarget() and Proxy::GetHandler()
in chakrashim. This enables util.types.isProxy() and util.inspect()
with the showProxy flag as true to function correctly in
node-chakracore.

Also enable related tests for node-chakracore:
* parallel/test-util-types.js
* parallel/test-util-inspect.js

Fixes: nodejs#488
Fixes: nodejs#523
PR-URL: nodejs#525
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Jack Horton <Jack.Horton@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compatibility modified test
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chakrashim: implement Proxy methods rhuanjl/node-chakracore
4 participants
@kfarnung @jackhorton @MSLaguana @rhuanjl