JSON.stringify without toJSON

[ljharb]

Correcting the tests - For JSON.stringify to ignore symbols, it must work without Symbol.prototype.toJSON.

[zloirock]

You serious? :)

[ljharb]

Absolutely. The spec doesn't contain Symbol.prototype.toJSON - JSON.stringify tests for internal slots on Symbols. es6-shim overwrites the global JSON.stringify to comply with this test.

If you'd like me to wait while you update core-js I'll be happy to leave the results as "true" before merging.

[zloirock]

Anyway, it can't be merged because contains part of another subtest - conversion to object - V8 fails ({})[Object(Symbol())], if you don't know, and contains typo - obSym. Please, test anything before adding.

[ljharb]

1. Thanks, I'll fix that now. I'd tested, and then cleaned up the variable names, and clearly didn't test after that minor refactor.
2. I wasn't aware that ({})[Object(Symbol())] threw in v8 - I'll fix that as well, thanks.

[ljharb]

Tested in v8; the issues you brought up are fixed.

[zloirock]

Tested in v8

Uncaught illegal access ;)

BTW I've been going replace fixing JSON.stringify with Symbol in core-js from .toJSON method to wrapping JSON.stringify, but because of this V8 and some other bugs I don't see any reason do it now.

[ljharb]

@zloirock what version? Works fine for me in v45 on Mac.

[zloirock]

Windows, Chrome 45, Node 4.1, Opera 32, many other ;) Mac versions too.

As minimum, on JSON.stringify(Object(Symbol())).

BTW, it should be "{}", not undefined - and it requires correct conversion to object behavior - another subtest.

Strange, perhaps, your method of testing if you don't see it.

[ljharb]

Weird - I see Chrome failing on var sym = Symbol(); Object(sym) == sym but I don't see any exceptions in the console, and the JSON.stringify test passes (same on Opera 32). Can you help debug by pointing to which line is throwing that exception?

How are you testing node v4.1 exactly?

My test in the commit already does JSON.stringify(object) === '{}' where object has a symbol-valued key, and an object symbol key, so I'm not sure where you see undefined.

I'm testing by simply opening the compilers file for es6-shim in the given browsers - can you provide a screenshot? This might be a difference on Windows versus Mac.

[ljharb]

I just verified in Chrome 45 on Windows, as well as Opera 32, via browserstack. Is there a chance you're not checking out the right SHA to verify this with? Note that you have to run npm install && npm run build:compilers every time your working directory changes, since the compiler builds aren't checked into source control, and to make sure you've got the latest version of es6-shim.

Thanks for the typo, and the Chrome tip, those were helpful review comments!

[zloirock]

I already shown where (and, btw, not only here),
.

My test in the commit already does JSON.stringify(object) === '{}' where object has a symbol-valued key, and an object symbol key, so I'm not sure where you see undefined.

You didn't see last assert (JSON.stringify(sym) === undefined)?

[ljharb]

@zloirock i think the spec requires that JSON.stringify(Symbol()) returns undefined. See http://www.ecma-international.org/ecma-262/6.0/#sec-json.stringify which leads to http://www.ecma-international.org/ecma-262/6.0/#sec-serializejsonproperty which leads to step 12, which says Return undefined.

[ljharb]

What does s.js contain? Testing methods should be located in the repo so that others can validate and share them.

[zloirock]

actually the spec requires that JSON.stringify(Symbol()) returns undefined.

For JSON.stringify(Symbol()), not JSON.stringify(Object(Symbol())) (2 of 3 your "tests").

What does s.js contain?

Your code. What else? :)

[ljharb]

Ah, thanks for clarifying. In that case, it will still go to http://www.ecma-international.org/ecma-262/6.0/#sec-serializejsonproperty - where if there's no callable toJSON method (the default for Symbol), it falls right through step 5, leading it to step 12, which still returns undefined. So, by my reading, JSON.stringify(Object(Symbol())) must still, by spec, return undefined - thus my test is correct.

Thanks again for the two errors you spotted - if I'm reading the spec wrong, can you clarify?

[zloirock]

still, by spec, return undefined hence my test is correct.

Wrong. By spec, it's equal JSON.stringify({}) -> "{}" (step 11d), reread spec.

[ljharb]

Thanks, you're correct - I'll update the test and fix es6-shim as well. I'll also split it up into a subtest so it's more apparent what's passing and failing.

[ljharb]

k, updated - now symbol objects are a separate test. I verified each of the results manually. I didn't test babel/typescript specifically; please let me know if these should pass (how do you go about testing these? i don't see an html file prepared for it).

[zloirock]

The problem here - es6-shim and core-js results. You mark es6-shim result as "Yes", but it will pass the test only with native Symbol. I can change fixing JSON.stringify with Symbol in core-js from .toJSON method to wrapping JSON.stringify and test will be passed with native Symbol. But not with the polyfill - Object(symbol) === symbol. So how should be marked core-js? :) Related #641. My point - both - es6-shim and core-js - "No" with the comment "works with native symbols".

[ljharb]

That's a good point. Whether it's "no, but works with native symbols" or "yes, but requires native symbols" (I think the latter is more accurate for both), a flag might be appropriate here.

However, since this test is about JSON.stringify, not Symbols, if core-js also wraps the stringify method such that it works with native symbols, I'd say both should be marked the same - true, with a flag indicating that native Symbol support is required.

[kangax]

So... the disagreement is on whether to mark tests as "Yes" or "No" with appropriate comments?

[zloirock]

Disagreement in #641, before resolving this issue I agree with results from the current PR and have no ideas why it's (sure, with adding core-js to results) still not merged.

[ljharb]

@kangax the real question is whether shims that ensure proper support for a feature in an env where it's natively available should get a "Yes" for supporting it, or a "No" for not providing the full semantics the feature depends on. If, as I maintain, things like "Object.assign also enumerates symbols" should be a "Yes" even if it's only true in an env with native symbols, then I'll proceed to update this PR to include core-js's true results, and we'll also be able to resolve #641 and other similar questions in the future.

[kangax]

I haven't done much work in this area so I might be missing something but I would think shims' results should be entirely independent of the environment. Meaning, we don't want to say "Yes" when shim works only with certain features available in the environment. My reasoning is that from the user perspective — as someone looking at a table to find out shim support of certain features — you don't want to care about underlying environments; you want to see how shim behaves across all of them, even when they have wildly varying support. Of course this might paint an unrepresentative and skewed picture, which is why I've been wanting to add support for overlaying/combining columns to see actual support (e.g. Firefox + es6-shim). But meanwhile, I'd err on a side of "No" with a note.

What do you think?

/cc @webbedspace @jdalton

[ljharb]

I don't think that's as useful, tbh. I'd prefer a Yes with a note, just like all the notes that are "Yes with a flag" or "Yes with function "name" properties to be natively configurable" etc.

I totally agree that being able to select an engine and shims/polyfills, and see the combined results, is very useful. But, when looking at the "shim" column, I'm interested in the unqualified "no"s - ie, what things it doesn't support at all - and the "No with flag" I think distracts from that.

[kangax]

I hear ya and of course this is subjective. I'm not strongly opposed to either of these so ultimately I'd love to know what users of the table want. Otherwise, we (maintainers) can just all vote for one or the other.

[jdalton]

I think the shims should be applied to a common environment. Shims don't hold much value in environments where they aren't needed. With MS effectively dropping IE < 11 support on Jan 12th it might be a good idea to set the environment bar a bit higher (IE 11).

[ljharb]

When a test is typeof Symbol === 'function' or typeof Symbol.iterator === 'symbol', then sure, the shims should all fail one or both, because the latter is unshimmable. But when it's "Object.assign copies symbols correctly", then "correctly", in a browser without symbols, is "no symbols are copied", as long as in a browser with symbols, "symbols are copied" - that's what's useful to know, since we can't know in advance what a compat table user's browser support matrix will be.

[jdalton]

Symbols tests, and those related, should be false in environments without symbol support. Basing the results on a single environment, like IE 11, would help anchor them.

[zloirock]

When a test is typeof Symbol === 'function' or typeof Symbol.iterator === 'symbol', then sure, the shims should all fail one or both, because the latter is unshimmable.

I strongly disagree and with core-js + babel 6 both those tests are passed so it will not change anything.

Symbols tests, and those related, should be false in environments without symbol support.

You mean also all for-of, destructuring, etc results for compilers which work everywhere? :) My point - only if it will not work in an ES5-only environment (like currently in the README).

[jdalton]

My point - only if it will not work in an ES5-only environment

I'm saying the chart should reflect support from a single environment running the transpiled code.
If that means symbol and proxy tests fail then so be it.

[zloirock]

@jdalton one more time - baseline environment - IE9 or IE11 hasn't native Symbol, Symbol-related features like for-of or spread works fine for compilers with default polyfills, but you propose disable results because environment hasn't native Symbol? :)

[zloirock]

With MS effectively dropping IE < 11 support on Jan 12th

Are you sure? Someone still uses Vista :)

[jdalton]

disable results because environment hasn't native Symbol? :)

Anything that's not shimmable gets a fail. I also think the chart as it is, with typescript+core-js and babel+core-js, is a bit overloaded. I rather see TypeScript and Babel alone, then maybe a column just for core-js.

Are you sure? Someone still uses Vista

Yep, that's why I qualified my statement with "effectively". Vista is gearing up for its end of life in 2017. That combined with its low popularity is why I'm not so concerned with it.

[zloirock]

@jdalton babel column without core-js and "not shimmable" (by someone's point of view) features definitely interesting solution - little more than 20%. Without "not transpilable" features like class (required new.target, [[Construct]], etc) - little more than 10%. Community definitely will appreciate it :)

[afmenez]

What is the point of having multiple versions of the same browser here? Isn't "firefox36:true" supposed to work for all versions >=36?

[ljharb]

The goal was to mark a version that's explicitly tested. Browsers do occasionally (both unintentionally and intentionally) change their support of a behavior, so it's useful to have explicitly checked.

[zloirock]

@ljharb any plans about this PR? It's opened about a year.

[ljharb]

I'm still waiting for a consensus on whether tests that are useless in environments without native Symbol should pass, fail, or be marked N/A. I think "fail" is the wrong approach, and I see the argument for "pass" being wrong too.

In this specific case, I'd argue that a test for "ignore Symbol primitives" can't possibly fail when the entire environment ignores Symbol primitives, so I think that test should automatically be marked passing in an environment without native Symbols.

[zloirock]

@ljharb as I already wrote - before this consensus makes sense to merge it in the current form. Sure, with adding results core-js and actual browsers :)

[ljharb]

OK - I'll update this PR in the next week or so with up-to-date results, and merge in with that understanding.