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

Explicitly bind wrapping IIFE to window for ES5 strict mode compatibility #1398

Merged
merged 2 commits into from
Apr 9, 2018

Conversation

patrickkunka
Copy link
Contributor

@patrickkunka patrickkunka commented Apr 6, 2018

I came across the following issue while using Shaka as a dependency of another pre-bundled player library that was delivered to my company as a single, minified UMD-wrapped package without a package.json and therefore no resolvable dependencies. Due to the nature of a UMD library, all dependencies are wrapped inside it so that it can be used as a <script> tag without the need for a build process. I anticipate many other video players may deliver Shaka to customers like this to avoid complexity for their customers.

I will be following up with the authors of the player to see if they can provide a commonjs build instead with a resolvable list of dependencies, but I can't see any downside of adding the suggested changes below and making Shaka more robust in the process.

Thanks!

Description

Shaka assumes that this within the top-level IIFE will be bound to window in a browser environment.

(function() {
    console.log(this); // window
})()

This works correctly as long as Shaka is a top-level dependency (node_module) of an ES6+ project and therefore ignored from babel transpilation. However, when it is part of some other bundle which is transpiled from ES6 to ES5 before being consumed, this process will add the following:

function() { // wrapping webpack module closure
    'use strict';
    ...
    (function() { // shaka UMD module
        console.log(this); // undefined
    })()
}

As per the strict-mode specification, this is undefined within an implicitly called IIFE.

Shaka is then broken, as various pieces of code such as window.console become lookups on undefined.

To address this, we can explicitly bind the IIFE to the window, and Shaka will function correctly regardless of whether it is being evaluated in a strict mode context or not.

(function() {
    console.log(this); // window
}).call(window)

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@joeyparrish joeyparrish added this to the v2.4 milestone Apr 6, 2018
@joeyparrish joeyparrish added the type: enhancement New feature or request label Apr 6, 2018
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all makes sense to me. I'm going to run tests on our build bot. You will also need to fill out a Contributor License Agreement and respond to the CLA bot before I am allowed to merge this.

Thanks!

@shaka-bot
Copy link
Collaborator

All tests passed!

@patrickkunka
Copy link
Contributor Author

Thanks! I've signed the CLA.

@googlebot
Copy link

CLAs look good, thanks!

@patrickkunka
Copy link
Contributor Author

patrickkunka commented Apr 6, 2018

Excellent, thanks for the fantastic library by the way, I've been using for years across various projects/companies without issue and found the documentation top notch. Unfortunately this was the first time I've been forced to use it in a pre-packaged context like this.

@joeyparrish
Copy link
Member

Thank you for the kind words. We're glad to be doing this. And thanks for the contribution as well!

I forgot one thing, though. You will need to add yourself to the CONTRIBUTORS file (in alphabetical order), and you'll need to add an entry to the AUTHORS file, as well. If you signed the CLA as an individual, you add yourself to AUTHORS. If you are covered by a corporate CLA, you add your company to AUTHORS.

Thanks!

@ghost
Copy link

ghost commented Apr 7, 2018

Have now added myself to AUTHORS and CONTRIBUTORS. Should be good to merge!

@joeyparrish joeyparrish merged commit 8743bcf into shaka-project:master Apr 9, 2018
@joeyparrish
Copy link
Member

Thank you for your contribution!

@joeyparrish
Copy link
Member

Cherry-picked to v2.3.6.

@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 type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants