Add test for passing Symbol to JS and fix it for GraalJSRuntime

[eregon]

@byroot Could you review and merge? :)

@bjfish noticed barber and/or Discourse is relying on this.

For curiosity I also tried what happens if we pass Object.new to JS and on miniracer & node it just passes it as a String with .to_s like JSON.dump does (e.g. "#<Object:0x0000000001d05718>"). On duktape it gives TypeError: cannot convert Object and on the Graal.js backend it gives TypeError: Unknown how to convert to JS: #<Object:0x1ad8>.
I think the TypeError is good there, JSON just using to_s seems rather unexpected in general, so I think let's keep that as-is.

[casperisfine]

Looks good. I'll merge on green.

[eregon]

Rhino fails the new test, cc @josh.

And it seems all the conversion is done on the rhino gem side:

execjs/lib/execjs/ruby_rhino_runtime.rb

Line 35 in 39118e2

unbox @rhino_context.eval(properties).call(*args)

So we do we fix it with an explicit .map { |value| Symbol === value ? value.to_s : value } there, or do we skip this test on Rhino, or something else?
We can also report it to https://github.com/rubyjs/therubyrhino

[eregon]

Upstream issue: rubyjs/therubyrhino#43

[casperisfine]

So we do we fix it with an explicit .map { |value| Symbol === value ? value.to_s : value } there, or do we skip this test on Rhino, or something else?

ExecJS aims at maximum consistency between the various implementations, so yes I think the Rhino adapter should cast symbols to string. That's on us though, it's fine for Rhino to have support for Symbols if it wants to. So I'd close the upstream issue.

[eregon]

There are already two tests skipped on Rhino: test_call_with_this and test_babel.
So I think better to skip it on Rhino for now (with the issue URL) and when a decision upstream is taken we can revisit (I think it makes sense to handle Symbols in therubyrhino, if we do it here it's less efficient and potentially redundant).

[casperisfine]

I think it makes sense to handle Symbols in therubyrhino

I doesn't no. ExecJS goal is to be a common denominator of all available JS implementations. It shouldn't allow you to use features that that vast majority of implementations don't support.

If people wish to use these features they should use the implementation directly, not ExecJS.

Now I'm ok with just skipping the test as Rhino is far from a popular backend, but it's not an official feature of ExecJS and we might remove it at any point.

[eregon]

It's your decision, are you OK with the skip or should I change the Rhino backend for now?

I didn't consider it but if Symbol is supported upstream we can remove the workaround here if there wasn't a release in between (and worst case it's redundant but wouldn't change behavior).

It shouldn't allow you to use features that that vast majority of implementations don't support.

All ExecJS backends except therubyrhino support it with this PR and my view is the therubyrhino backend will support it in the future, either from upstream or with the extra .map here.

[casperisfine]

are you OK with the skip or should I change the Rhino backend for now?

I'm OK with both, however if you decide to go with the skip, it shouldn't point to the upstream issue because it's not a Rhino issue, it's an ExecJS issue, so it's basically a TODO.

[eregon]

Alright, I'll just add the change in the backend, and link to the issue there saying it might not be necessary in the future anymore

[casperisfine]

it might not be necessary in the future anymore

I'm not sure we understand each others. Rhino like a few other JS implementation support passing ruby specific types to the JS engine. I very much doubt they'll see this as a bug, I'm 99% sure it's a feature.

The reason we don't want to allow this in ExecJS is because many adapters can't support it, e.g. the NodeJS adapter is limited to JSON types because it has to serialize the objects and pass them through STDIN / argv.

[eregon]

Right, I didn't think about it like that.

MiniRacer, all external runtimes (including node), duktape, Graal.js convert Symbol to String when passing them to JS (I can't run javascriptcore/jscript locally).
I consider GraalVM to already have this behavior because TruffleRuby already converts Symbols to Strings when passing them to another language, at least conceptually since there is interop for strings but not symbols (few languages have those).
So this seemed to me the most sensible behavior since that's what basically all backends do.

But it's true recent JS has Symbols nowadays (although with wildly different semantics, notably they are not unique for a given name), and Rhino doesn't support that and does something different which is to wrap it like any other Object.

In any case, I updated the PR to convert in the rhino backend, that seems best for compatibility between runtimes in ExecJS.

[eregon]

In general ExecJS follows the semantics of JSON.generate & JSON.parse, because that's what the external runtime need to do.
(this also explains why functions are replaced by nil when going to Ruby, it's also what JSON.stringify() does in JS)
In those semantics, the only choice for Symbols is to convert them to JSON strings or error (or null), and converting to JSON strings seems more useful and is already relied upon by ExecJS usages.

[casperisfine]

This isn't quite enough thoug. args could be [{foo: 42}], so you'd need something that walk down the object graph.

An easy fix could be JSON.parse(JSON.dump(args)).

[eregon]

Oh, of course, not sure how I missed that.
I'll do that then, it should provide better compatibility for Rhino, at the cost of some performance but then anyway it's not a very common JS runtime nowadays (very old JS version).

[eregon]

Done, I used JSON.parse(JSON.generate(args), create_additions: false) for consistency with ExternalRuntime, and added tests like this nested example.

[byroot]

at the cost of some performance

Yeah, it's gonna be a bit slower, but ExecJS is really not what you want to use if you want the best performance.