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

feat(build system): Added step to build process that creates index.js in... #35

Merged
merged 2 commits into from
Mar 17, 2015

Conversation

palmerj3
Copy link
Contributor

... the root folder and exports all the main classes for CommonJS support

Change-Id: I53e0bcf483f6e3d46e9d9a28a4881fad7b6be13c

… in the root folder and exports all the main classes for CommonJS support

Change-Id: I53e0bcf483f6e3d46e9d9a28a4881fad7b6be13c
@palmerj3 palmerj3 mentioned this pull request Mar 11, 2015
@joeyparrish
Copy link
Member

Since index.js is generated code, would you please add it to .gitignore?

Change-Id: Ife5365fa334b72c682a7da9666df2487e800083e
@joeyparrish
Copy link
Member

Sorry for the delay, I missed the update. Thanks for adding this!

joeyparrish added a commit that referenced this pull request Mar 17, 2015
feat(build system): Added step to build process that creates index.js in...
@joeyparrish joeyparrish merged commit d10a512 into shaka-project:master Mar 17, 2015
@palmerj3 palmerj3 deleted the addModuleExports branch March 17, 2015 19:21
@palmerj3
Copy link
Contributor Author

No worries thanks! Any idea when these latest changes will be published to NPM?

@joeyparrish
Copy link
Member

With our next release version, 1.3.0, which I'm currently expecting by the end of the month.

Oh, and one more thing I forgot to ask you to add to the pull request before I merged it. Can you please add yourself to CONTRIBUTORS and AUTHORS?

@palmerj3
Copy link
Contributor Author

Ok cool thanks! I've submitted a separate PR for the CONTRIBUTORS and AUTHORS update. #37

@sanbornhilland
Copy link
Contributor

@palmerj3
Do you know if it's straightforward to take your build and use it as an AMD module? Ideally I'd like to import shaka using RequireJS. I've attempted to take the output of this build task and wrap it in a requireJS define() which works to a point. I can import it but then when I try to play back content I get an error: cannot read property addEventListener of undefined.

For some reason I'm having a hard time finding out exactly where it's failing but from what I can tell it seems like there might be a problem when it hits FakeEventTarget and tries to create shaka.util.MultiMap. shaka seems to be undefined at this point.

Do you have any insight here? Or at the very least, could you explain how you are using it? This might give me a better idea of what I have to do to get it working how I want.

@palmerj3
Copy link
Contributor Author

palmerj3 commented Apr 9, 2015

@sanbornhnewyyz I think instead of us supporting all possible async dependency resolution solutions you could utilize webpack or something like this within your project which would allow you to utilize CommonJS, RequireJS, etc dependencies interchangeably.

Sounds similar to what you've already done but perhaps take a look at:
http://requirejs.org/docs/api.html#cjsmodule

For me the way I'm using it is:

var shaka = require('shaka-player').shaka;
shaka.polyfill.Fullscreen.install();
shaka.polyfill.MediaKeys.install();
shaka.polyfill.VideoPlaybackQuality.install();

var player = new shaka.player.Player(myVideoDomNode);
var playerSource = new shaka.player.DashVideoSource(mpdUrl, null);
player.load(playerSource);

@palmerj3
Copy link
Contributor Author

palmerj3 commented Apr 9, 2015

@sanbornhnewyyz I spoke too soon... so I was able to get it working but I'm not sure it would be wise to include in the main build... @joeyparrish would like your thoughts.

It's not ideal because it's tying the build process to specific implementation details (e.g. shaka.player, shaka.polyfill, etc). But it works... perhaps it's useful to you.

If you add this to build.sh:
(library_sources_0; closure_sources_0) | compile_0 \ --output_wrapper='%output% define({player: shaka.player, polyfill: shaka.polyfill, util: shaka.util});' \ --js_output_file "$dir"/index2.js

In this case "index2.js" will define all the AMD stuff needed so you can do something like this:

(function() {
  'use strict';

  require(['node_modules/shaka-player/index2'], function(shaka) {
    shaka.polyfill.Fullscreen.install();
    shaka.polyfill.MediaKeys.install();
    shaka.polyfill.VideoPlaybackQuality.install();

    var player = new shaka.player.Player(document.getElementById('foo'));
    var playerSource = new shaka.player.HttpVideoSource('http://video.webmfiles.org/elephants-dream.webm');
    player.load(playerSource).then(function() {
      player.play();
    });
  });
}());

@palmerj3
Copy link
Contributor Author

palmerj3 commented Apr 9, 2015

@joeyparrish @sanbornhnewyyz I created this - let me know your thoughts. #55

@sanbornhilland
Copy link
Contributor

@palmerj3 maybe I spoke too soon. At least for requireJS purposes, your commonJS build works fine if you wrap the whole thing with:

define(function (require, exports, module) {...});

I tried doing this with your example above and it playsback fine. But when I try to require this module in our actual project I run into these undefined errors when I try to playback content. So it looks like something is not playing nice with our project

I agree with not supporting all module flavours though for the big ones you could add a flag that let's you set which module type you want.

@joeyparrish
Copy link
Member

I wouldn't mind supporting both commonJS and requireJS by default, however I'd prefer it greatly if we could combine all of this into one wrapper. It cuts down compilation time, which makes a big difference for us trying to iterate rapidly on new features. Let's continue this discussion on #55.

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants