Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safari forOf and spread issues for Set object. #1810

Open
patrick-martin-shoutpoint opened this issue Mar 11, 2015 · 9 comments
Open

Safari forOf and spread issues for Set object. #1810

patrick-martin-shoutpoint opened this issue Mar 11, 2015 · 9 comments

Comments

@patrick-martin-shoutpoint

When using the Set object, the following code fails in Safari 8 desktop and mobile:
"Error: undefined is not a function (evaluating 'f.next()')"

var set = new Set([1,2,3,4]);

var arr = [...set]; // FAIL

for (var v of set) // FAIL
  console.log(v);

var vals = set.values();
for (var v of vals) // FAIL
  console.log(v);

Note: This code fails to compile in the Traceur REPL on Safari.
https://google.github.io/traceur-compiler/demo/repl.html#var%20set%20%3D%20new%20Set(%5B1%2C2%2C3%2C4%5D)%3B%0A%0Avar%20arr%20%3D%20%5B...set%5D%3B%0A%0Afor%20(var%20v%20of%20set)%0A%20%20console.log(v)%3B%0A%0Avar%20vals%20%3D%20set.values()%3B%0Afor%20(var%20v%20of%20vals)%0A%20%20console.log(v)%3B%0A

spread@https://google.github.io/traceur-compiler/bin/traceur.js:1316:38

getAnonymousModule@https://google.github.io/traceur-compiler/bin/traceur.js:681:34
eval code
eval@[native code]
evaluateCodeUnit@https://google.github.io/traceur-compiler/bin/traceur.js:29800:36
evaluate@https://google.github.io/traceur-compiler/bin/traceur.js:29956:50
evaluate@https://google.github.io/traceur-compiler/bin/traceur.js:30257:37
handleCodeUnitLoaded@https://google.github.io/traceur-compiler/bin/traceur.js:30170:24
module@https://google.github.io/traceur-compiler/bin/traceur.js:30056:32
module@https://google.github.io/traceur-compiler/bin/traceur.js:30317:41
transcode
compileContents
compile
wrappedFunc
@caitp
Copy link
Contributor

caitp commented Mar 11, 2015

yeah, iterator protocol in JSC is still a work in progress. last I checked (in jan/feb), @@iterator wasn't exposed at all in JS, and was only implemented for specific types (just arrays, maybe).

maybe it would make sense to always use the polyfilled version for JSC?

@arv
Copy link
Collaborator

arv commented Apr 2, 2015

We should probably be a lot stricter when it comes to determining when to use the polyfills. One suggestion is to use the polyfills if there is no [Symbol.iterator].

@cwalther
Copy link

cwalther commented Apr 9, 2015

I just ran into a similar issue with Safari on iOS 8.1.3. The problematic cases in my case were for-of over an Array as well as construction of a Map from an Array (or iterable). The specific reasons were that

  • Safari natively implements Array.prototype.entries and Array.prototype.keys, but the iterators returned by them lack a next method.
  • Safari natively implements Map, but the constructor does not take an argument.

I can work around the problems by including the following as the first script of the page:

// Remove insufficient built-in implementations so that the polyfills from traceur-runtime kick in.
if (Array.prototype.entries && typeof [1].entries().next !== 'function') {
    delete Array.prototype.entries;
}
if (Array.prototype.keys && typeof [1].keys().next !== 'function') {
    delete Array.prototype.keys;
}
try {
    new Map([]);
}
catch(e) {
    delete Map;
}

I assume these checks could be built into the polyfillArray and polyfillMap functions but have not tried that.

@arv
Copy link
Collaborator

arv commented Apr 9, 2015

Safari natively implements Array.prototype.entries and Array.prototype.keys, but the iterators returned by them lack a next method.

WHAT!! How do you iterate over them then? How can they ship something so broken... makes me very sad.

@cwalther What does Map.length return in Safari? If it says 0 we should be able to use that instead of using the try/catch.

@cwalther
Copy link

I have no idea what the purpose of these iterators is, they appear to have no methods (or other properties) at all besides the ones inherited from Object.

Map.length indeed returns 0. (I wasn’t even aware that functions had a length property, thanks for pointing me to that!)

Interestingly, Set.length is 0 as well, but new Set([4, 5]) still works (Set([4, 5]) on the other hand throws TypeError: Set does not accept arguments when called as a function).

@arv
Copy link
Collaborator

arv commented Apr 10, 2015

TypeError: Set does not accept arguments when called as a function

Sad. So misleading. Set throws when called.

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-set-iterable

Step 1

@cwalther
Copy link

I just see in that document that Map.length should be 0, unless I’m misunderstanding something, so maybe checking for that is not a suitable way of detecting insufficient implementations after all.

By the way, es6-shim also fixes the issues I had on that version of Safari, and it has a whole bunch of criteria for checking whether to replace Map, maybe that’s a more reliable source. (I didn’t use es6-shim as a final solution because I was wary of potential unintended interactions between es6-shim and traceur-runtime.)

@arv
Copy link
Collaborator

arv commented Apr 10, 2015

You are correct. Map.length should be 0.

I think a suitable test is to see if Map.prototype[Symbol.iterator] is a function.

@zloirock
Copy link

@cwalther @arv bad idea. es6-shim also has a problem - it's replace collections in all modern environment. Maybe it's partly fixed after yesterday's reworking. My point about collections polyfilling here.

arv added a commit to arv/traceur-compiler that referenced this issue May 17, 2015
Safari does not support ES6 Map/Set correctly which leads to trouble.
Be more strict in detecting whether the native Map/Set is good enough.

Fixes google#1650, google#1810
arv added a commit to arv/traceur-compiler that referenced this issue May 18, 2015
Safari does not support ES6 Map/Set correctly which leads to trouble.
Be more strict in detecting whether the native Map/Set is good enough.

Fixes google#1650, google#1810
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants