Bug 215047 – JSClassRef should work with JS class syntax.

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

JSClassRef should work with JS class syntax and should be retrievable via API

[Keith Miller]

Created attachment 405764 [details]
Patch

[Geoffrey Garen]

Comment on attachment 405764 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405764&action=review

> Source/JavaScriptCore/API/tests/testapi.cpp:659
> +    definition.callAsConstructor = constructor;

Is this code reachable? I think this would only have an effect if you did "new (new SuperClass)"

[Radar WebKit Bug Importer]

<rdar://problem/66708727>

[Keith Miller]

Created attachment 407201 [details]
Patch

[Keith Miller]

Created attachment 407223 [details]
Patch

[Darin Adler]

Comment on attachment 407223 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407223&action=review

> Source/JavaScriptCore/ChangeLog:15
> +        the new.target. Ideally we would have passed the derived classes
> +        constructor from the beginning of our support for JS subclassing
> +        but at this point that's probably not compatible with too many
> +        applications.

We can create an old compatible version and a new version designed the way we like. We don’t have to give up on it forever.

> Source/JavaScriptCore/API/APICallbackFunction.h:91
> +        // If we are doing a derived class construction get the .prototype property off the new target first so we look behave closer to normal JS.

"look behave" -> "behave"?

> Source/JavaScriptCore/API/APICallbackFunction.h:119
> +        // This won't trigger proxy traps on newObject's prototype handler but that's probably desirable here anyway.

Add test cases covering this?

> Source/JavaScriptCore/API/JSObjectRef.h:343
> +When setting callAsConstructor it should be noted that it is not possible to support JS subclassing via the extends clause of JS class syntax. This is because there is no backwards compatible way to vend the derived class's constructor to callAsConstructor. If you intend to subclass it's recomended to use JSObjectMakeConstructor.

Spelling error: "recomended".

Not sure I understand the meaning of the phrase "backwards compatible" here.

The last sentence probably shouldn’t use the word "you". Maybe the sentence should have a structure more like "JSObjectMakeConstructor <can do it>."

> Source/JavaScriptCore/API/tests/testapi.cpp:54
> +    APIString(String string)

I suggest const String&.

> Source/JavaScriptCore/API/tests/testapi.cpp:55
> +        : APIString(string.ascii().data())

I suggest utf8() instead of ascii(), since ascii() is intended for debugging only. Not great even for tests.

> Source/JavaScriptCore/API/tests/testapi.cpp:654
> +    JSClassDefinition definition = kJSClassDefinitionEmpty;

static const maybe?

> Source/JavaScriptCore/API/tests/testapi.cpp:660
> +    auto constructor = [] (JSContextRef ctx, JSObjectRef, size_t, const JSValueRef*, JSValueRef*) -> JSObjectRef {
> +
> +        return JSObjectMake(ctx, jsClass, nullptr);
> +    };

Stray blank line.

> Source/JavaScriptCore/API/tests/testapi.cpp:662
> +    JSObjectRef superClass = JSObjectMakeConstructor(context, jsClass, constructor);

Superclass could be a single word instead of "super class".

> Source/JavaScriptCore/API/tests/testapi.cpp:664
> +    ScriptResult result = callFunction("(function (SuperClass) { class SubClass extends SuperClass { method() { return 'value'; } }; return new SubClass(); })", superClass);

Ditto. Superclass and subclass.

> Source/JavaScriptCore/API/tests/testapi.cpp:667
> +    JSObjectRef subClass = const_cast<JSObjectRef>(result.value());

Ditto.

[Keith Miller]

Comment on attachment 407223 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407223&action=review

>> Source/JavaScriptCore/ChangeLog:15
>> +        applications.
> 
> We can create an old compatible version and a new version designed the way we like. We don’t have to give up on it forever.

Yeah, I was thinking about just adding a new class attribute to enable it. Maybe, that's a better way to do this?

>> Source/JavaScriptCore/API/APICallbackFunction.h:91
>> +        // If we are doing a derived class construction get the .prototype property off the new target first so we look behave closer to normal JS.
> 
> "look behave" -> "behave"?

Fixed.

>> Source/JavaScriptCore/API/JSObjectRef.h:343
>> +When setting callAsConstructor it should be noted that it is not possible to support JS subclassing via the extends clause of JS class syntax. This is because there is no backwards compatible way to vend the derived class's constructor to callAsConstructor. If you intend to subclass it's recomended to use JSObjectMakeConstructor.
> 
> Spelling error: "recomended".
> 
> Not sure I understand the meaning of the phrase "backwards compatible" here.
> 
> The last sentence probably shouldn’t use the word "you". Maybe the sentence should have a structure more like "JSObjectMakeConstructor <can do it>."

Fixed.

It kinda has two meanings. So the ideal solution is probably that we would have the allocated object already for you. Unfortunately, that's not binary compatible. We could also add it but again it's a pretty substantial API change, that I'm not sure is worth it. The second option/meaning would be to pass the derived class's constructor to the callback instead of the base class's constructor. But that may break existing code, maybe anyway but the user may consider that code broken already so 🤷‍♂️... but even then the API client would need to get the derived class's .prototype and set the __proto__ themselves, which isn't intuitive. 

Unfortunately, the same trick that we use for JSObjectMakeConstructor doesn't really work here since I'd expect that the user is allocating an object with a different JSClassRef. Although, maybe that's not the case?

I changed the last sentence to "Subclassing is supported via the JSObjectMakeConstructor function however."

>> Source/JavaScriptCore/API/tests/testapi.cpp:54
>> +    APIString(String string)
> 
> I suggest const String&.

Done.

>> Source/JavaScriptCore/API/tests/testapi.cpp:55
>> +        : APIString(string.ascii().data())
> 
> I suggest utf8() instead of ascii(), since ascii() is intended for debugging only. Not great even for tests.

Done.

>> Source/JavaScriptCore/API/tests/testapi.cpp:654
>> +    JSClassDefinition definition = kJSClassDefinitionEmpty;
> 
> static const maybe?

I doubt it matters but sure.

[Darin Adler]

Comment on attachment 407223 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407223&action=review

>>> Source/JavaScriptCore/API/JSObjectRef.h:343
>>> +When setting callAsConstructor it should be noted that it is not possible to support JS subclassing via the extends clause of JS class syntax. This is because there is no backwards compatible way to vend the derived class's constructor to callAsConstructor. If you intend to subclass it's recomended to use JSObjectMakeConstructor.
>> 
>> Spelling error: "recomended".
>> 
>> Not sure I understand the meaning of the phrase "backwards compatible" here.
>> 
>> The last sentence probably shouldn’t use the word "you". Maybe the sentence should have a structure more like "JSObjectMakeConstructor <can do it>."
> 
> Fixed.
> 
> It kinda has two meanings. So the ideal solution is probably that we would have the allocated object already for you. Unfortunately, that's not binary compatible. We could also add it but again it's a pretty substantial API change, that I'm not sure is worth it. The second option/meaning would be to pass the derived class's constructor to the callback instead of the base class's constructor. But that may break existing code, maybe anyway but the user may consider that code broken already so 🤷‍♂️... but even then the API client would need to get the derived class's .prototype and set the __proto__ themselves, which isn't intuitive. 
> 
> Unfortunately, the same trick that we use for JSObjectMakeConstructor doesn't really work here since I'd expect that the user is allocating an object with a different JSClassRef. Although, maybe that's not the case?
> 
> I changed the last sentence to "Subclassing is supported via the JSObjectMakeConstructor function however."

These comments are for software developers who are using our API.

I think we need to be more specific and explicit than the phrase "backward compatibility" to help them understand. It’s kind of "inside baseball" to talk about what we can and can’t do compatibly. Instead we should just document what the function does without making excuses or offering rationale for why it doesn’t work a different way.

The comments don’t need to explain why it’s designed the way it is — they do need to explain clearly how to use this correctly and what to expect.

[Keith Miller]

Comment on attachment 407223 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407223&action=review

>>>> Source/JavaScriptCore/API/JSObjectRef.h:343
>>>> +When setting callAsConstructor it should be noted that it is not possible to support JS subclassing via the extends clause of JS class syntax. This is because there is no backwards compatible way to vend the derived class's constructor to callAsConstructor. If you intend to subclass it's recomended to use JSObjectMakeConstructor.
>>> 
>>> Spelling error: "recomended".
>>> 
>>> Not sure I understand the meaning of the phrase "backwards compatible" here.
>>> 
>>> The last sentence probably shouldn’t use the word "you". Maybe the sentence should have a structure more like "JSObjectMakeConstructor <can do it>."
>> 
>> Fixed.
>> 
>> It kinda has two meanings. So the ideal solution is probably that we would have the allocated object already for you. Unfortunately, that's not binary compatible. We could also add it but again it's a pretty substantial API change, that I'm not sure is worth it. The second option/meaning would be to pass the derived class's constructor to the callback instead of the base class's constructor. But that may break existing code, maybe anyway but the user may consider that code broken already so 🤷‍♂️... but even then the API client would need to get the derived class's .prototype and set the __proto__ themselves, which isn't intuitive. 
>> 
>> Unfortunately, the same trick that we use for JSObjectMakeConstructor doesn't really work here since I'd expect that the user is allocating an object with a different JSClassRef. Although, maybe that's not the case?
>> 
>> I changed the last sentence to "Subclassing is supported via the JSObjectMakeConstructor function however."
> 
> These comments are for software developers who are using our API.
> 
> I think we need to be more specific and explicit than the phrase "backward compatibility" to help them understand. It’s kind of "inside baseball" to talk about what we can and can’t do compatibly. Instead we should just document what the function does without making excuses or offering rationale for why it doesn’t work a different way.
> 
> The comments don’t need to explain why it’s designed the way it is — they do need to explain clearly how to use this correctly and what to expect.

Sure, I'll remove part of the comment then.

[Keith Miller]

Created attachment 407405 [details]
Patch for landing

[EWS]

Committed r266236: <https://trac.webkit.org/changeset/266236>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407405 [details].