module.exports issue since 6.2.0

[thijstriemstra]

The problem is the combination of ES6 exports export default videojs and requirejs imports require('video.js)

video.js switched from module.exports = videojs in 6.1.0 to export default videojs in 6.2.0
And while videojs-record still uses require('video.js) and if you use ES6 stops working.

https://github.com/videojs/video.js/blob/v6.1.0/src/js/video.js#L727
https://github.com/videojs/video.js/blob/v6.2.0/src/js/video.js#L726

So the fix to this is as I understand it to switch to ES6 imports or to change the require to require('video.js).default

But I'm not really an expert regarding this subject.

Explanations of this problem:
#2698 (comment)
https://medium.com/@kentcdodds/misunderstanding-es6-modules-upgrading-babel-tears-and-a-solution-ad2d5ab93ce0

More background info: collab-project/videojs-record#149

[gkatsev]

I've been thinking a lot about this, thanks for opening it here to remind me.
One of the main changes in 6.2.0 was switching from a browserify-based build system to a rollup-based build system. Previously, if you were using a bundler yourself, it would then look through the module tree itself and bundle it together and then we used browserify to generate a UMD build that got put into dist/ on publish.
With rollup, we now bundle all the video.js codebase into one, excluding dependencies. It's used to generate a single CJS, ES, and UMD file. Meaning, one file that uses commonjs syntax for getting dependencies (require('foo')), one file for using ecmascript module syntax (import foo from 'foo';), and one that was the old UMD equivalent.
With this, we changed the package.json to point the regular old main file at dist/video.cjs.js, but we always added a new pkg.module property to point at the new dist/video.es.js. This is so that users of bundlers that are ESM aware have some extra benefits.
However, it seems like this has created a lot of issues because the Video.js ecosystem has not largely moved to ES module syntax yet as you've experienced. It seems to me like these bundlers provide the video.es.js to modules that use ESM syntax and video.cjs.js to those that use require syntax and of course that causes issues because it means that plugins have a different version of Video.js than the actually app that depends on video.js as well.
So, you get something like:

app
├─ video.es.js
└─ videojs-record
   └─ video.cjs.js

But you want something like

app
├─ video.es.js
└─ videojs-record

So, a potential solution is to just remove pkg.module from our package.json. Thankfully, we don't really have many dependencies, especially dependencies that others rely on, so, we don't have a lot of benefit from having a video.es.js. And since we are generating a video.cjs.js, we still get some of the benefits that rollup provides.
Given that this broke some things, I think it would be reasonable to make this change as another patch onto 6.2.

What are your thoughts @videojs/core-committers?

[heff]

Yeah, if this was an ecosystem breaking change and it doesn't hurt badly to fix that, I'd be in favor of the patch. Then revisiting with 7.0 and if it's worth the breaking change at that point, drawing a clear line in the sand, an plugins can report if they're 7.0+ compatible. 👍

[misteroneill]

Sounds good to me.