paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 388 forks source link

Shimmed Symbol.iterator is not recognized #459

Closed wintlu closed 4 years ago

wintlu commented 5 years ago

When we include a Symbol shim before es6-shim, Symbol.iterator key is not correctly recognized, and this caused bunched of other issues. (for example, cannot retrieve correct iterator from ArrayIterator object)

To reproduce:

  1. open this link in IE11: https://wintlu.github.io/es6-shim-symbol-repo.html (source code here)
  2. Check error is thrown

Solution: Check symbol value for native and shimmed respectively:

      var isNativeSymbol = function isNativeSymbol(value) {
        return typeof globals.Symbol === 'function' && typeof x === 'symbol';
      }

      var isShimSymbol = function isShimSymbol(value) {
        return typeof globals.Symbol === 'function' && Object(x) instanceof globals.Symbol;
      }

      var Type = {
        symbol: function (x) {
          return isNativeSymbol(x) || isShimSymbol(x);
        }
      }

Refer to my sample PR before: https://github.com/wintlu/google-map-es6-shim/pull/1/files

Want to confirm if my understanding and solution is correct, as always, many thanks to @lencioni

ljharb commented 5 years ago

Symbols can not be shimmed - only shammed. As such, I'd suggest not trying to include them.

If you do, you'd have to include a symbol sham after es6-shim (that may not actually work either, now that I think about it).

wintlu commented 5 years ago

Hi @ljharb , I see you reopened this issue, so we will need more discussion here? actually the solution I pasted is from core-js: https://github.com/zloirock/core-js/blob/master/packages/core-js/modules/es.symbol.js#L88 So would it make sense to have such check? Thank you so much for the help!

ljharb commented 5 years ago

I closed it by accident, is all.

No - it would be incorrect and inappropriate to use anything but typeof to check for symbols, imo. Additionally, instanceof doesn’t work across realms.

wintlu commented 5 years ago

thanks Jordan for the explanation! I agree with your point here Right now I am facing an issue that seems either need to be fixed in google maps API or es6-shim side, could you provide me some suggestions for the fix? @ljharb @lencioni

Context:

  1. We need to upgrade to google map js API 3.37 or 3.38 by end of Oct. 2019, (it will auto update if we don't do anything), IE11 will break if we don't fix the issue
  2. These Google maps js API adds Symbol.iterator shim in IE11,

Issue:

  1. Google maps JS API detects if array supports iterator, using Symbol.iterator, in below code, a is an array object. image
  2. Because es6-shim doesn't recognize Symbol.iterator, so error will happen and google maps warns about it export:This site overrides Array.from() with an implementation that doesn't support iterables, which could cause Google Maps JavaScript API v3 to not work correctly

To reproduce follow the link for original error page and fixed page, open in IE11 https://es6-shim-google-maps.glitch.me/

Solution 1

  1. Add symbol-es6 or similar shims before es6-shim
  2. Fix es6-shim as suggested above

Actually other people had the similar issue and solved it similar way (because core-js already has Symbol polyfill and the suggested fix, so it's quite simple for them) https://github.com/zloirock/core-js/issues/567#issuecomment-497626013

Solution 2 Ask google to fix their code I think it will be hard, first the process of fixing in google maps is slow. Second I think it is not an easy fix on google maps: They polyfilled Symbol and used it widely. It seems reasonable to check array's iterator by a[Symbol.iterator]... I am not sure how to suggest them for a reasonable fix

We need to solve this issue by end of October to avoid IE11 break, do you have suggestion for any other options? THANKS A LOT!!

ljharb commented 5 years ago

To be honest, I think asking Google is appropriate. es6-shim and core-js are the most common shims in use; if Google Maps won't work with es6-shim, it's going to break a lot of websites.

wintlu commented 5 years ago

Thanks Jordan, discussed with @lencioni, I will ask Google if they can fix it or provide suggestions.

To clarify, core-js is not breaking with Google Maps because:

  1. They have Symbol polyfill
  2. They have the logic to check polyfilled Symbol

These 2 points are the Solution 1 I proposed above for using es6-shim, so it's a really simple fix in zloirock/core-js#567: just load the polyfill before Google Maps

ljharb commented 5 years ago

If it's truly not possible for Google Maps to fix their code before the end of October (please do pursue this regardless), then I think it's reasonable to add your solution to es6-shim - but I would strongly prefer not having to make the shim code less correct solely due to Google forcing the issue with broken code.

wintlu commented 4 years ago

Google Maps fixed their code and our site was upgraded to latest google maps, thanks!!

ljharb commented 4 years ago

@wintlu hooray, glad to hear it! Does that mean #460 is no longer needed?

wintlu commented 4 years ago

Yeah!! I will close it now, thank you so much for the supporting!