Rollup: Specify AMD id

[nicjansma]

Description

Today, video.js exports itself as an AMD module (if the site supports AMD) via the UMD header:

(function (global, factory) {
  typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
  typeof define === 'function' && define.amd ? define(factory) :
  (global = typeof globalThis !== 'undefined' ? globalThis : global || self, global.videojs = factory());
})(this, (function () { 'use strict';

The second clause which runs if AMD/RequireJS is active, is a define(factory) function call. This is an "anonymous" definition of the module. Being anonymous will break on sites using the lightweight RequireJS shim called almond, which expect all modules to be named.

This can be challenging if video.js is being pulled into the site via a third-party, i.e. as part of ads.

Prior to using Browserify/Rollup, video.js had implemented a change to ensure define('videojs', factory) was used:

#1844

However, after the switch to Browserify, that small change was lost:

#3826

This error is seen by others, i.e. #1843 (comment)

Specific Changes proposed

This PR simply ensures video.js is named for AMD via the rollup.config.js.

The UMD definition changes to this:

(function (global, factory) {
  typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
  typeof define === 'function' && define.amd ? define('videojs', factory) :
  (global = typeof globalThis !== 'undefined' ? globalThis : global || self, global.videojs = factory());
})(this, (function () { 'use strict';

Requirements Checklist

• Feature implemented / Bug fixed
• If necessary, more likely in a feature request than a bug fix
  • Change has been verified in an actual browser (Chrome, Firefox, IE)
  • Unit Tests updated or fixed
  • Docs/guides updated
  • Example created (starter template on JSBin)
  • Has no DOM changes which impact accessiblilty or trigger warnings (e.g. Chrome issues tab)
  • Has no changes to JSDoc which cause npm run docs:api to error
• Reviewed by Two Core Contributors

[welcome]

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

• Run npm run lint -- --errors locally to catch formatting errors earlier.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[gkatsev]

Can this change break folks that are currently using AMD? I think that would be my only concern with a change like this.

[nicjansma]

@gkatsev I don't believe this is a breaking change at all. This is how video.js was until ~6.2.* as well.

The Rollup code just uses that new id option, and makes sure the generated code has the name specified.

[gkatsev]

Sure, but if someone has amd set up and working with 8.17.3, and we release this change, would they need to change their configuration for it to work again or would it not matter and just would mean less configuration for folks going forward again?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.79%. Comparing base (f0db8f1) to head (bdd3f39).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8843      +/-   ##
==========================================
+ Coverage   83.18%   83.79%   +0.60%     
==========================================
  Files         120      120              
  Lines        8047     8047              
  Branches     1931     1931              
==========================================
+ Hits         6694     6743      +49     
+ Misses       1353     1304      -49     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nicjansma]

I wasn't sure about this until I built a bunch of test cases! Here's what I've found:

Using define(factory) (existing behavior):

• ✅ video.js direct inclusion
• ✅ video.js with RequireJS loaded as require(['lib/videojs'], ...)
• ✅ video.js with RequireJS loaded as require(['videojs'], ...) (require path override specified)
• ✅ video.js with RequireJS loaded as require(['lib/videojs'], ...) using r.js optimizer
• ✅ video.js with RequireJS loaded as require(['videojs'], ...) using r.js optimizer (require path override specified)
• ✅ video.js with RequireJS loaded as require(['lib/videojs'], ...) using almond
• ✅ video.js with RequireJS loaded as require(['videojs'], ...) using almond (require path override specified)

Using define('videojs', factory) (this PR):

• ✅ video.js direct inclusion
• ❌ video.js with RequireJS loaded as require(['lib/videojs'], ...)
• ✅ video.js with RequireJS loaded as require(['videojs'], ...) (require path override specified)
• ❌ video.js with RequireJS loaded as require(['lib/videojs'], ...) using r.js optimizer
• ✅ video.js with RequireJS loaded as require(['videojs'], ...) using r.js optimizer (require path override specified)
• ❌ video.js with RequireJS loaded as require(['lib/videojs'], ...) using almond
• ✅ video.js with RequireJS loaded as require(['videojs'], ...) using almond (require path override specified)

So unfortunately it looks like by forcing the module id to 'videojs' in the define(...), it will break existing cases where video.js is being included at a specified (relative) path such as require(['lib/videojs'])

<html>
    <head>
        <link href="lib/video-js.css" rel="stylesheet">
        <script data-main="main" src="lib/require.js"></script>
        <!-- video.js is at lib/videojs.js -->
    </head>
    <body>
        <script>
            require(['lib/videojs'], function(videojs) {
                // videojs will be undefined if built with the AMD id of `videojs`
        </script>
    </body>
</html>

In these cases, the file lib/videojs.js is fetched, but the file itself only defines videojs. So callback var of the require() statement is undefined.

The cases where it continues to work are where the user had already specified the module as videojs and just used a RequireJS path override to point to where that file is, e.g. lib/videojs.js.

<html>
    <head>
        <link href="lib/video-js.css" rel="stylesheet">
        <script data-main="main" src="lib/require.js"></script>
        <!-- video.js is at lib/videojs.js -->
    </head>
    <body>
        <script>
            requirejs.config({
                paths: {
                    'videojs': 'lib/videojs'
                },
            });
            require(['videojs'], function(videojs) {
                // videojs is OK here
        </script>
    </body>
</html>

So I'm not sure where that leaves this PR. This would potentially be a breaking change for some.

It would be nice to fix this though. I came across this issue when my ad provider, who is using video.js, started to include it in their bundle. On my site that uses Almond (which requires a name for all modules), video.js wouldn't load.

[gkatsev]

we could provide another build that includes it, but the main build would need to wait for a breaking change.