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

ReferenceError: 'Symbol' is undefined in IE 11 #1455

Closed
rounce opened this issue May 30, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@rounce
Copy link

commented May 30, 2018

Test on demo: https://shaka-player-demo.appspot.com/demo/#asset=https://storage.googleapis.com/shaka-demo-assets/angel-one/dash.mpd;lang=ru-RU;build=compiled
image
This error appeared in commit b27ea82
There's no issue when build=debug_compiled

Perhaps you need to add a polyfill and describe it in the readme.

@ismena

This comment has been minimized.

Copy link
Member

commented May 30, 2018

@joeyparrish Could you take a look if you have a chance? (The mentioned commit is yours and I don't have access to IE today). If not, I'll check this out later this week.

@joeyparrish

This comment has been minimized.

Copy link
Member

commented May 30, 2018

Will do.

@joeyparrish joeyparrish self-assigned this May 30, 2018

@joeyparrish

This comment has been minimized.

Copy link
Member

commented May 30, 2018

The issue appears in the release build, but not the debug build.

To dig into this, I started by examining the compiled code and looking for Symbol. The debug and release builds both have this, but it looks a little different.

I tried to put a breakpoint in the code, but IE was making that difficult and wouldn't let me see the compiled bundle, possibly because there was a source map. The code that is breaking, though, was inserted by the compiler and doesn't map back to anything in the uncompiled library sources.

To better debug, I edited the compiled library and used http://jsbeautifier.org/ to format the first several lines of the compiled code, including the code that polyfills Symbol. My plan was then to add console logs to help me debug it.

While doing this, though, I noticed that only the release build uses a module wrapper. I commented out the module wrapper, and the problem went away. So it has something to do with the interaction between the Closure Compiler's inserted polyfill and our module wrapper.

@joeyparrish joeyparrish added bug and removed needs triage labels May 30, 2018

@joeyparrish

This comment has been minimized.

Copy link
Member

commented May 30, 2018

The code inserted by the compiler doesn't work in a wrapper. It does not appear to be our wrapper's fault.

In some places, the compiler-inserted code refers to $jscomp.global.Symbol (which works), and in others, it refers to Symbol (which only works without the wrapper or on browsers with native Symbol support). This mistake appears in the definitions of $jscomp.makeIterator, $jscomp.generator.Generator_, and $jscomp.iteratorFromArray. This bug appears to be present in the latest Closure Compiler sources.

I will patch it locally in our pre-built compiler, then create a PR to send the changes to the Closure Compiler team.

@joeyparrish

This comment has been minimized.

@ismena

This comment has been minimized.

Copy link
Member

commented May 30, 2018

@joeyparrish Boy, am I glad I asked you 😛

@shaka-bot shaka-bot closed this in 962ec0a May 30, 2018

joeyparrish added a commit that referenced this issue Jun 1, 2018

Patch Closure Compiler to fix polyfill+wrapper
When using a module wrapper, polyfills inserted by the compiler do not
go to "window", so all internal references to "Symbol" from those
polyfills must go through "$jscomp.global".  This patches the compiler
to fix this issue.

Upstream: google/closure-compiler#2957
Closes #1455

Change-Id: Ifb9a4f651f31c3099d660c4b818da8457d89dbbc

joeyparrish added a commit that referenced this issue Jun 1, 2018

Patch Closure Compiler to fix polyfill+wrapper
When using a module wrapper, polyfills inserted by the compiler do not
go to "window", so all internal references to "Symbol" from those
polyfills must go through "$jscomp.global".  This patches the compiler
to fix this issue.

Upstream: google/closure-compiler#2957
Closes #1455

Change-Id: Ifb9a4f651f31c3099d660c4b818da8457d89dbbc
@joeyparrish

This comment has been minimized.

Copy link
Member

commented Jun 1, 2018

Cherry-picked for v2.3.9 and v2.4.1.

shaka-bot pushed a commit that referenced this issue Jun 20, 2018

Fix output wrapper for Electron, ESM, and Closure
This updates our wrapper in several ways:
  - Adds comments and whitespace and meaningful variable names
  - Makes the CommonJS loader compatible with Electron, fixing #1463
  - Adds a "global" parameter to the inner scope, fixing #1455
  - Strips whitespace and comments from the wrapper during the build

The wrapper is much more readable and debuggable, which should help
maintain support for these various module loaders in the future.

Tested and working with the new wrapper:
  - CommonJS (NodeJS, etc.)
  - AMD (requirejs)
  - Electron (loads into global scope without loader, no longer
    conflicts with CommonJS)
  - ES modules (partially supported, loads into global scope)

Since the wrapper is now fixed in a way that avoids #1455 and
google/closure-compiler#2957, this also reverts
"Patch Closure Compiler to fix polyfill+wrapper",
commit 962ec0a.

Closes #1463

Change-Id: I5d3ac056cb8db04dc714afee0cb069f2a45a456d

joeyparrish added a commit that referenced this issue Jun 29, 2018

Fix output wrapper for Electron, ESM, and Closure
This updates our wrapper in several ways:
  - Adds comments and whitespace and meaningful variable names
  - Makes the CommonJS loader compatible with Electron, fixing #1463
  - Adds a "global" parameter to the inner scope, fixing #1455
  - Strips whitespace and comments from the wrapper during the build

The wrapper is much more readable and debuggable, which should help
maintain support for these various module loaders in the future.

Tested and working with the new wrapper:
  - CommonJS (NodeJS, etc.)
  - AMD (requirejs)
  - Electron (loads into global scope without loader, no longer
    conflicts with CommonJS)
  - ES modules (partially supported, loads into global scope)

Since the wrapper is now fixed in a way that avoids #1455 and
google/closure-compiler#2957, this also reverts
"Patch Closure Compiler to fix polyfill+wrapper",
commit 962ec0a.

Closes #1463

Change-Id: I5d3ac056cb8db04dc714afee0cb069f2a45a456d

joeyparrish added a commit that referenced this issue Jun 29, 2018

Fix output wrapper for Electron, ESM, and Closure
This updates our wrapper in several ways:
  - Adds comments and whitespace and meaningful variable names
  - Makes the CommonJS loader compatible with Electron, fixing #1463
  - Adds a "global" parameter to the inner scope, fixing #1455
  - Strips whitespace and comments from the wrapper during the build

The wrapper is much more readable and debuggable, which should help
maintain support for these various module loaders in the future.

Tested and working with the new wrapper:
  - CommonJS (NodeJS, etc.)
  - AMD (requirejs)
  - Electron (loads into global scope without loader, no longer
    conflicts with CommonJS)
  - ES modules (partially supported, loads into global scope)

Since the wrapper is now fixed in a way that avoids #1455 and
google/closure-compiler#2957, this also reverts
"Patch Closure Compiler to fix polyfill+wrapper",
commit 962ec0a.

Closes #1463

Change-Id: I5d3ac056cb8db04dc714afee0cb069f2a45a456d

@shaka-bot shaka-bot added the archived label Jul 29, 2018

@google google locked and limited conversation to collaborators Jul 29, 2018

rounce pushed a commit to rounce/shaka-player that referenced this issue Jul 8, 2019

Patch Closure Compiler to fix polyfill+wrapper
When using a module wrapper, polyfills inserted by the compiler do not
go to "window", so all internal references to "Symbol" from those
polyfills must go through "$jscomp.global".  This patches the compiler
to fix this issue.

Upstream: google/closure-compiler#2957
Closes google#1455

Change-Id: Ifb9a4f651f31c3099d660c4b818da8457d89dbbc
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.