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

Unable to load shaka-player.compiled.js in Electron 2 #1463

Closed
radiantmediaplayer opened this issue Jun 13, 2018 · 12 comments

Comments

Projects
None yet
3 participants
@radiantmediaplayer
Copy link

commented Jun 13, 2018

Have you read the FAQ and checked for duplicate open issues?
Y

What version of Shaka Player are you using?:
2.4.0

Can you reproduce the issue with our latest release version?:
Y

Can you reproduce the issue with the latest code from master?:
Not tried

Are you using the demo app or your own custom app?:
custom

If custom app, can you reproduce the issue using our demo app?:
N/A

What browser and OS are you using?:
Electron 2.0.2 for Windows 10 x64
Chromium 61.0.3163.100

What are the manifest and license server URIs?:
https://storage.googleapis.com/shaka-demo-assets/angel-one/dash.mpd

What did you do?
Try to run the basic usage example at https://shaka-player-demo.appspot.com/docs/api/tutorial-basic-usage.html in an Electron 2.0 app for Windows 10.

What did you expect to happen?
It should work

What actually happened?
It did not work

I see in the Chromium console:
Uncaught ReferenceError: shaka is not defined
at HTMLDocument.initApp (index.html:14)

This happens when I try to include the shaka-player.compiled.js file, either from cdnjs or locally.
The weirdest thing is that this issue does not happen when I include shaka-player.compiled.debug.js either from cdnjs or locally. I have verified that shaka-player.compiled.js works fine in Google Chrome and can be loaded.

Note that this issue is not about using the FILE protocol with shaka.net.NetworkingEngine.registerScheme but loading the compiled version of Shaka player in an Electron app.
We are also working with other libs in Electron (hls.js, three.js) and Shaka player is the only lib we are seeing this issue with. At this stage my best guess is that there is some sort of incompatibility due to the Shaka player build process (maybe some closure thing?) that causes shaka-player.compiled.js to not work in an Electron 2 app.
I don't know if you officially support Electron, or if this could be of interest to the community, but any hint would be appreciate.

@radiantmediaplayer

This comment has been minimized.

Copy link
Author

commented Jun 14, 2018

@joeyparrish thanks for the reply. Here are the results:
v2.4.0, uncompiled: https://shaka-player-demo.appspot.com/demo/#build=uncompiled

loads ok

v2.4.0, debug: https://shaka-player-demo.appspot.com/demo/#build=debug_compiled

loads ok

v2.4.0, release: https://shaka-player-demo.appspot.com/demo/#build=compiled

FAILS to load and print the following error in console
Uncaught ReferenceError: shaka is not defined
at shakaDemo.init (main.js:102)

nightly, uncompiled: https://nightly-dot-shaka-player-demo.appspot.com/demo/#build=uncompiled

loads ok

nightly, debug: https://nightly-dot-shaka-player-demo.appspot.com/demo/#build=debug_compiled

loads ok

nightly, release: https://nightly-dot-shaka-player-demo.appspot.com/demo/#build=compiled

FAILS to load and print the following error in console
Uncaught ReferenceError: shaka is not defined
at shakaDemo.init (main.js:102)

One very easy way to reproduce it, is to install https://github.com/electron/electron-quick-start and then go to main.js and replace mainWindow.loadFile('index.html') with mainWindow.loadURL('https://shaka-player-demo.appspot.com/demo/#build=compiled') and then run npm start. I have verified that the issue is the same when packaging an Electron app with electron-packager.

EDIT: I just tried with Electron 2 and Ubuntu 16.04 LTS with the above URLs and the results are the same as when testing in Windows 10.

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

Thanks for the details on how to use electron. Very helpful! I'll take a look.

@joeyparrish joeyparrish added bug and removed needs triage labels Jun 14, 2018

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

I can reproduce the issue. It looks like it has something to do with the output wrapper we use with the compiler. It is meant to allow Shaka to be used with module loaders, but it seems that Electron has a "module" global variable that is confusing the wrapper.

@shaka-bot shaka-bot added this to the v2.5 milestone Jun 14, 2018

@radiantmediaplayer

This comment has been minimized.

Copy link
Author

commented Jun 15, 2018

Thanks for looking into it. Since Electron runs a local node server, all API and variables available with node are available in the window of the Chromium view generated, including global node variables like module or process.

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Jun 15, 2018

I'm struggling to build a single output wrapper that works in all of these environments:

  1. CommonJS (supported already)
  2. AMD (supported already)
  3. NodeJS (not well supported, but sort of works)
  4. Electron (not yet supported, wrapper is confusing this with CommonJS/NodeJS)

And in a perfect world, I'd some day add:

  1. ES module support

I'll let you know as I make progress.

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Jun 15, 2018

Correction: CommonJS is equivalent to NodeJS format. (Why is the module loader landscape so messy?)

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Jun 18, 2018

I think I have something working in all cases. I need to clean it up and get it through code review, but I see Electron, nodejs, CommonJS, AMD, and ES modules all working. I have also been able to revert an earlier change made to the compiler to fix #1455.

@joeyparrish joeyparrish self-assigned this Jun 18, 2018

@radiantmediaplayer

This comment has been minimized.

Copy link
Author

commented Jun 19, 2018

I went into this a bit further and I found that the issue I reported also exists with other libs like jQuery that are module compatible.
A solution is described here and I have verified this works with Shaka (I have not covered all use-cases yet like DRM, but it works for basic playback). This is what I did to make it work:

window.shaka = require('./rmp/dash/shaka-player.compiled.js');

Since both window and module are defined in Electron we need to re-attach shaka global variable to the window object (the shaka compiled lib cannot be included with a script tag as of now, we need to use require). If you have a long-term out-of-the-box solution this is great, but in the meantime the above could be used as a workaround.

@shaka-bot shaka-bot closed this in 1d27c29 Jun 20, 2018

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

This has been fixed in the master branch, and the fix will be cherry-picked to our next releases. Sorry for the inconvenience!

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

This comment has been minimized.

Copy link
Member

commented Jun 29, 2018

Cherry-picked for v2.4.2.

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

This comment has been minimized.

Copy link
Member

commented Jun 29, 2018

Cherry-picked for v2.3.10.

@shaka-bot shaka-bot added the archived label Aug 19, 2018

@google google locked and limited conversation to collaborators Aug 19, 2018

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.