bytecodealliance / javy

JS to WebAssembly toolchain
Apache License 2.0
2.1k stars 101 forks source link

Leverage QuickJS's `JS_ToString` in logging #644

Closed jbourassa closed 2 months ago

jbourassa commented 2 months ago

The problem

Javy crashes when console.loging certain JavaScript values.

Description of the change

Use QuickJS's JS_ToString implementation for console.{log,error} instead of rolling our own; reverting to what Javy 2.x did.

This change also adds new serialization test cases to capture the current behaviour. Symbols are excluded because they crashed on Javy 2.x and throw an exception with this branch (from this QuickJS line). See this repro script and outputs.

Tradeoffs

I suspect this is slower because we'll allocate Strings for any values. At the same time, we get to leverage QuickJS' implementation just like we did prior to the introduction of the rquickjs bindings.

The alternative is to re-implement printing each types, thus choosing a string representation for each type. If we want to go that route, we have to choose wether we want to follow what other engines do (more work, slower), or stick with Javy's current minimal approach.

Checklist

saulecabrera commented 2 months ago

Also, it's not entirely clear to me why Symbols are excluded in QuickJS itself. By checking the behavior in other engines, I can confirm that they are correctly stringified as Symbol(v) where v is the symbol's description field. Could we perhaps special case symbols so the behavior matches?

jbourassa commented 2 months ago

✅ Symbol special-casing

Also, it's not entirely clear to me why Symbols are excluded in QuickJS itself.

Same. It looks intentional though, but probably not in the case of JS_ToString -- it probably makes sense for other scenarios.


✅ wtf8 support, the added test is a little crude but I think it works?

LMK what you think!