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

Proxy on clr #1645

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Proxy on clr #1645

merged 3 commits into from
Oct 13, 2023

Conversation

viruscamp
Copy link
Contributor

@viruscamp viruscamp commented Oct 12, 2023

fix #1643

provide detailed message for exceptions in JsProxy
donot use ReferenceEquals on data property value comparing

provide detailed message for exceptions in JsProxy
donot use ReferenceEquals on data property value comparing
@viruscamp
Copy link
Contributor Author

viruscamp commented Oct 12, 2023

Other exceptions in JsProxy should be attached messages, and make tests. It's so boring, I won't do it recently.

Next, I may rewrite PropertyDescriptor.IsDataDescriptor, as currently CLR field property method and etc are treat as data property, in JsProxy it triggers multiple property getter calls.

PropertyDescriptor.IsDataDescriptor should not have side effect, which is calling CustomValue gettter.

Jint/Native/Proxy/JsProxy.cs Outdated Show resolved Hide resolved
Jint/Native/Proxy/JsProxy.cs Outdated Show resolved Hide resolved
let p = new Proxy(o, handler);
""");
var ex = Assert.Throws<JavaScriptException>(() => _engine.Evaluate("p.value"));
AssertJsTypeError(_engine, ex, "'get' on proxy: property 'value' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '42' but got '32')");
Copy link
Contributor Author

@viruscamp viruscamp Oct 12, 2023

Choose a reason for hiding this comment

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

@lahma
Should we comment out all AssertJsTypeError?
As it depends internal implementations and unstable error messages.
Just comment out these for develop usage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you did great work replicating the V8 error messages and they can stay, we can always tweak later if comes a maintenance burden 👍🏻

improve `JsValue.SameValue` to compare `ObjectWrapper`
add a new test `ProxyHandlerGetDataPropertyShouldNotCheckClrType` for `JsString` and `ConcatenatedString`
Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

Great work! 👍🏻

@lahma lahma merged commit 48d512f into sebastienros:main Oct 13, 2023
3 checks passed
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.

JS Proxy on Clr Object
2 participants