juce-framework / JUCE

JUCE is an open-source cross-platform C++ application framework for desktop and mobile applications, including VST, VST3, AU, AUv3, LV2 and AAX audio plug-ins.
https://juce.com
Other
6.54k stars 1.73k forks source link

[Bug]: (JUCE8) Can't return an object containing methods in Javascript calls #1401

Closed benkuper closed 3 months ago

benkuper commented 3 months ago

Detailed steps on how to reproduce the bug

When calling a C function, if the return value is a var object containing method (using setMethod ), the fromVar() function triggers an assert and the method is not handled. When used in registerNativeObject, method var are supported indeed.

I guess this is a starting point but I'm lost on how to go from there, as choc::value::Value doesn't seem to be able to hold methods/functions.

     if (variant.isObject())
     {
         if (auto* dynamicObject = dynamic_cast<DynamicObject*> (variant.getObject()))
         {
             choc::value::Value value { choc::value::Type::createObject ("") };

             for (const auto& [name, prop] : dynamicObject->getProperties())
                 value.setMember (name.toString().toRawUTF8(), fromVar (prop));

             return value;
         }
     }

     if (variant.isMethod())
     {
         //what to put here ? seems way more complex than just adding a method
         return {};
     }

     if (variant.isUndefined())
         return {};

Am I missing a big point on the QuickJS transition ? Should I rewrite all my thousand lines to match a new data structure ? It still seems that DynamicObject is used in registerNativeObject, and calling functions that have been declared in DynamicObject and set using registerNativeObject work. But I'm lost when it comes to objects that have been returned by a js-to-c function call.

In my case, everything is part of one big tree structure, nothing is out of it. So ideally passing only pointer to the object would be perfect, but since it was not how the previous Engine worked, and there is little to no documentation on how the QuickJS wrapper works in JUCE, i'm lost :)

What is the expected behaviour?

All practice should at least be consistent, meaning that either everything should use the new JSCursor / JSObject system (that I didn't find any documentation for, and isn't used in the Javascript Example Demo), or var Method should be supported in the VariantConverter::fromVar() function

Operating systems

Windows

What versions of the operating systems?

11

Architectures

64-bit

Stacktrace

No response

Plug-in formats (if applicable)

No response

Plug-in host applications (DAWs) (if applicable)

No response

Testing on the develop branch

The bug is present on the develop branch

Code of Conduct

benkuper commented 3 months ago

My take is that choc::Value would need to be updated to support functions. Otherwise, a lot needs to be done in to work around it and the data flow would not be as elegant and consistent as now

reuk commented 3 months ago

Thanks for reporting. We've pushed a fix here: https://github.com/juce-framework/JUCE/commit/606a7bc552b0737d2dee8feca3a203f167c33662

benkuper commented 3 months ago

This is awesome, I'll check it out ! and looking at the fix, it doesn't seem that it was trivial at all, thank you very much.