ClojureScript

Make instances of js/Symbol printable

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test

Description

At the moment instances of js/Symbol are not printable. Trying to
print them or enter them in the REPL causes an exception.

cljs.user> (.for js/Symbol "react.element")
#object[TypeError TypeError: Cannot convert a Symbol value to a string]

Symbols are supported in all major browsers, except Internet Explorer and Nashorn.
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol

Activity

Hide
Roman Scherer added a comment -

The attached patch adds an additional clause to `pr-writer-impl` and
implements the printing of Symbol instances, in the same way as it is
done for other JavaScript objects. Here's an example of a printed
js/Symbol:

(.for js/Symbol "react.element")
;;=> #object[Symbol "react.element"]

@david: Regarding your point about shims, do you think the
implementation of `js-symbol-defined?`, which was used for the ES6
iterator support, is enough for this patch? I'm not too familiar with
JavaScript and not sure if this already addressed the "shim" issue.

Another thing I stumbled upon is, that my test currently generates a
compiler warning when using the default compiler options. The warning
is generated when compiling the following ClojureScript form:

(.for js/Symbol "react.element")

The following snippet shows the warning and the generated code from my
test:

WARNING - Keywords and reserved words are not allowed as unquoted
property names in older versions of JavaScript. If you are targeting
newer versions of JavaScript, set the appropriate language_in option.

try{var values__13328__auto__ = (function (){var x__6628__auto__ = cljs.core.pr_str.cljs$core$IFn$_invoke$arity$variadic(cljs.core.array_seq([Symbol.for("react.element")], 0));

I think this has nothing to do with this patch, but with the emitted
code not being legal Ecmascript 3, since "for" is a reserved word.

The warning goes away when changing the :language-in option to
something newer than Ecmascript 3, or doing something like this:

((gobj/get js/Symbol "for") "react.element")

So, the questions is: Should the ClojureScript compiler handle those
reserved words when they appear in a function call or property lookup?

If that's the case I would leave the warning in that patch, and open
another issue for this new problem.

What do you think?

Roman

Show
Roman Scherer added a comment - The attached patch adds an additional clause to `pr-writer-impl` and implements the printing of Symbol instances, in the same way as it is done for other JavaScript objects. Here's an example of a printed js/Symbol:
(.for js/Symbol "react.element")
;;=> #object[Symbol "react.element"]
@david: Regarding your point about shims, do you think the implementation of `js-symbol-defined?`, which was used for the ES6 iterator support, is enough for this patch? I'm not too familiar with JavaScript and not sure if this already addressed the "shim" issue. Another thing I stumbled upon is, that my test currently generates a compiler warning when using the default compiler options. The warning is generated when compiling the following ClojureScript form:
(.for js/Symbol "react.element")
The following snippet shows the warning and the generated code from my test:
WARNING - Keywords and reserved words are not allowed as unquoted
property names in older versions of JavaScript. If you are targeting
newer versions of JavaScript, set the appropriate language_in option.

try{var values__13328__auto__ = (function (){var x__6628__auto__ = cljs.core.pr_str.cljs$core$IFn$_invoke$arity$variadic(cljs.core.array_seq([Symbol.for("react.element")], 0));
I think this has nothing to do with this patch, but with the emitted code not being legal Ecmascript 3, since "for" is a reserved word. The warning goes away when changing the :language-in option to something newer than Ecmascript 3, or doing something like this:
((gobj/get js/Symbol "for") "react.element")
So, the questions is: Should the ClojureScript compiler handle those reserved words when they appear in a function call or property lookup? If that's the case I would leave the warning in that patch, and open another issue for this new problem. What do you think? Roman
Hide
Roman Scherer added a comment -

Ok, given advanced mode and renaming of symbols I think there's not much the ClojureScript compiler can do about this.
I think I'll change the test to use ((gobj/get js/Symbol "for") "react.element") to remove the warning.
https://github.com/google/closure-compiler/wiki/FAQ#i-get-invalid-property-id-errors-but-it-works-on-firefox

Show
Roman Scherer added a comment - Ok, given advanced mode and renaming of symbols I think there's not much the ClojureScript compiler can do about this. I think I'll change the test to use ((gobj/get js/Symbol "for") "react.element") to remove the warning. https://github.com/google/closure-compiler/wiki/FAQ#i-get-invalid-property-id-errors-but-it-works-on-firefox
Hide
David Nolen added a comment - - edited

ECMAScript 3 is not an issue for people who are ok with outputting ECMAScript 5. The goog.typeOf of check on "symbol" does not seem like it would work with Symbol shims, have you confirmed?

Show
David Nolen added a comment - - edited ECMAScript 3 is not an issue for people who are ok with outputting ECMAScript 5. The goog.typeOf of check on "symbol" does not seem like it would work with Symbol shims, have you confirmed?
Hide
Roman Scherer added a comment -

David, yes you are right about goog.typeOf, it returns "object" when
js/Symbol is implemented via shims. I updated the patch and tested it
against the following shim implementation in Nashorn so far.

Start a Nashorn REPL.

./script/nashornrepljs

js/Symbol does not exist in Nashorn yet.

(exists? js/Symbol)
;;=> false

Load core-js shim into Nashorn.

(js/load "https://raw.githubusercontent.com/zloirock/core-js/master/client/shim.min.js")
(print (.for js/Symbol "x"))
;;=> #object[Symbol "x"]

I gave up on this shim implementation because I got an error when
trying to load the minified js into Nashorn.

https://github.com/medikoo/es6-symbol

This is another shim implementation, but it only enhances js/Symbol
when it already exists, so not useful in the context of Nashorn.

https://github.com/paulmillr/es6-shim

Show
Roman Scherer added a comment - David, yes you are right about goog.typeOf, it returns "object" when js/Symbol is implemented via shims. I updated the patch and tested it against the following shim implementation in Nashorn so far. Start a Nashorn REPL.
./script/nashornrepljs
js/Symbol does not exist in Nashorn yet.
(exists? js/Symbol)
;;=> false
Load core-js shim into Nashorn.
(js/load "https://raw.githubusercontent.com/zloirock/core-js/master/client/shim.min.js")
(print (.for js/Symbol "x"))
;;=> #object[Symbol "x"]
I gave up on this shim implementation because I got an error when trying to load the minified js into Nashorn. https://github.com/medikoo/es6-symbol This is another shim implementation, but it only enhances js/Symbol when it already exists, so not useful in the context of Nashorn. https://github.com/paulmillr/es6-shim
Hide
Roman Scherer added a comment -

Which implementations do support js/Symbol?

JavaScriptCore, NodeJS, SpiderMonkey, Chrome and Firefox support
js/Symbol. Evaluating (instance? js/Symbol (.for js/Symbol "x")) on
those engines returns false. On all those implementations symbols
generated via (.for js/Symbol "x") are primitive values and can be
identified via typeof. See the "Using the typeof operator with
symbols" section in [1].

Nashorn and Rhino do not support js/Symbol.

[1] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol

Show
Roman Scherer added a comment - Which implementations do support js/Symbol? JavaScriptCore, NodeJS, SpiderMonkey, Chrome and Firefox support js/Symbol. Evaluating (instance? js/Symbol (.for js/Symbol "x")) on those engines returns false. On all those implementations symbols generated via (.for js/Symbol "x") are primitive values and can be identified via typeof. See the "Using the typeof operator with symbols" section in [1]. Nashorn and Rhino do not support js/Symbol. [1] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol
Hide
Roman Scherer added a comment -

Looking around I found that the exception caused by printing symbols
happens because the str [1] function uses implicit string conversion
to convert it's argument into a string. This is explained in the
"Coercing a symbol to string" section of [2]. One way to solve the
problem is to use (.toString x) instead.

(.join #js [(.for js/Symbol "x")] "")
;;=> TypeError: Symbol is not a constructor

(.toString (.for js/Symbol "x"))
;;=> "Symbol(x)"

[2] https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/core.cljs#L2695
[2] http://www.2ality.com/2014/12/es6-symbols.html

Show
Roman Scherer added a comment - Looking around I found that the exception caused by printing symbols happens because the str [1] function uses implicit string conversion to convert it's argument into a string. This is explained in the "Coercing a symbol to string" section of [2]. One way to solve the problem is to use (.toString x) instead.
(.join #js [(.for js/Symbol "x")] "")
;;=> TypeError: Symbol is not a constructor

(.toString (.for js/Symbol "x"))
;;=> "Symbol(x)"
[2] https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/core.cljs#L2695 [2] http://www.2ality.com/2014/12/es6-symbols.html
Hide
Roman Scherer added a comment -

(= (goog/typeOf x) "symbol") vs (instance? js/Symbol x)

Symbols are primitive types and must be constructed via
Symbol("x"). They can't be constructed via new Symbol("x"), trying
to do so raises a TypeError. This is explained in more detail in the
"Safety checks" section of [1]. The "Using the typeof operator with
symbols" section of [2] explains how to identify symbols. It has to be
done via typeof, because symbols are primitive types and not
instances of Symbol.

[1] http://www.2ality.com/2014/12/es6-symbols.html
[2] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol

Show
Roman Scherer added a comment - (= (goog/typeOf x) "symbol") vs (instance? js/Symbol x) Symbols are primitive types and must be constructed via Symbol("x"). They can't be constructed via new Symbol("x"), trying to do so raises a TypeError. This is explained in more detail in the "Safety checks" section of [1]. The "Using the typeof operator with symbols" section of [2] explains how to identify symbols. It has to be done via typeof, because symbols are primitive types and not instances of Symbol. [1] http://www.2ality.com/2014/12/es6-symbols.html [2] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol
Hide
Roman Scherer added a comment - - edited

I think we have 2 options now to fix exceptions when printing symbols:

  • Change str to handle symbols as well.
  • Implement custom printing for symbols.

I think I would go for the first option. I think calling str on a
symbol can't be seen as implicitly coercing a symbol to a string, I
would say this is as explicit as it can get and we don't have to raise
an exception in this case. I think str is basically the Clojure
equivalent to toString in JavaScript, at least this is what the out
of date doc string of str says.

The file CLJS-1628-fix-str has an implementation of the first option.

Show
Roman Scherer added a comment - - edited I think we have 2 options now to fix exceptions when printing symbols:
  • Change str to handle symbols as well.
  • Implement custom printing for symbols.
I think I would go for the first option. I think calling str on a symbol can't be seen as implicitly coercing a symbol to a string, I would say this is as explicit as it can get and we don't have to raise an exception in this case. I think str is basically the Clojure equivalent to toString in JavaScript, at least this is what the out of date doc string of str says. The file CLJS-1628-fix-str has an implementation of the first option.
Hide
Francis Avila added a comment - - edited

Changing str to use .toString is not possible because of CLJS-847: there are unconfirmable reports that .toString() causes TypeError on Safari 6.0.5.

This problem has spilled over into the unresolved CLJS-890. This is a long thread, so see my recap comment, my failed efforts to reproduce the bug and my proposed final patch, which can be easily extended for the symbol case with a new "symbol" clause in the case statement.

Show
Francis Avila added a comment - - edited Changing str to use .toString is not possible because of CLJS-847: there are unconfirmable reports that .toString() causes TypeError on Safari 6.0.5. This problem has spilled over into the unresolved CLJS-890. This is a long thread, so see my recap comment, my failed efforts to reproduce the bug and my proposed final patch, which can be easily extended for the symbol case with a new "symbol" clause in the case statement.
Hide
Roman Scherer added a comment -

Thanks Francis, I'll take a look at this discussion.

Show
Roman Scherer added a comment - Thanks Francis, I'll take a look at this discussion.
Hide
Roman Scherer added a comment -

Since one can still make so called Symbol wrapper objects via the
Object() function [1], I added one more test case for this:

(js/Object (.for js/Symbol "x"))

Also the updated patch defines the test case only if js/Symbol exists?
in the JavaScript engine.

[1] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol

Show
Roman Scherer added a comment - Since one can still make so called Symbol wrapper objects via the Object() function [1], I added one more test case for this:
(js/Object (.for js/Symbol "x"))
Also the updated patch defines the test case only if js/Symbol exists? in the JavaScript engine. [1] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol
Hide
Mike Fikes added a comment -

Patch no longer applies.

Show
Mike Fikes added a comment - Patch no longer applies.

People

Vote (12)
Watch (7)

Dates

  • Created:
    Updated: