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

Use named exports in ESM output #5930

Merged
merged 1 commit into from
Nov 6, 2023
Merged

Use named exports in ESM output #5930

merged 1 commit into from
Nov 6, 2023

Conversation

robwalch
Copy link
Collaborator

@robwalch robwalch commented Oct 24, 2023

This PR will...

Include named exports in ESM output for classes and enums in addition to the default export.

Why is this Pull Request needed?

Allows import and use of classes and enums in the public API with ESM output. UMD output continues to use default Hls class export only for backwards compatibility.

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Resolves #5630

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@robwalch robwalch mentioned this pull request Oct 24, 2023
5 tasks
@robwalch robwalch marked this pull request as draft October 24, 2023 22:26
@robwalch robwalch force-pushed the feature/named-exports branch from 1846aac to 8497a3a Compare October 24, 2023 23:23
@robwalch robwalch marked this pull request as ready for review October 24, 2023 23:24
@robwalch robwalch changed the title Add named exports for classes and enums that we already export as types Use named exports in ESM output Oct 24, 2023
@robwalch robwalch force-pushed the feature/named-exports branch from 8497a3a to d84e9a8 Compare October 25, 2023 00:34
Comment on lines +18 to +22

export {
Hls,

Choose a reason for hiding this comment

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

@robwalch With this, npm run build:ci passes locally. I also replaced the dist folder into the node_modules/hls.js/dist folder of my project jellyfin-vue.

By changing this line: https://github.com/jellyfin/jellyfin-vue/blob/master/frontend/src/components/Playback/PlayerElement.vue#L33 to:

import Hls, { ErrorData, Events, ErrorTypes } from 'hls.js';

and fixing all the linter errors reported in that file, my project build without any issues,.

So doing this change is indeed the only required thing to be fully backwards-compatible.

Suggested change
export {
Hls,
export default Hls;
export {
Hls,

Choose a reason for hiding this comment

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

By the way, you can quickly test this in my project with GH Codespaces, since we have first-class support for devcontainers.

"Code" button at the right corner > Create Codespace

Just cd into the frontend directory and run npm run build to build. Go to the previously mentioned file where hls.js is imported and do the relevant tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fantastic! The mixed named and default exports was only a problem with the UMD output which we've given it's own target so it remains unchanged. So happy that the ESM output does not make the default export an object * with a default property.

@robwalch robwalch added this to the 1.5.0 milestone Oct 25, 2023
@robwalch robwalch requested a review from ferferga October 25, 2023 13:09
@robwalch robwalch force-pushed the feature/named-exports branch from 2ec50bb to 519a013 Compare October 25, 2023 13:10
@robwalch
Copy link
Collaborator Author

@cjpillsbury @littlespex when you have a chance, please file an issue re: composing player features/controllers.

The rough idea we discussed at day 1 of Demuxed is to have a constructor that required import and passing in of the controllers that handle the features you need, and 🤞 let tree-shaking handle removal of the ones you do not for a smaller footprint.

A good POC would need to replace most of the build constants we use today to define the light output with new entry files.

@ferferga
Copy link

@robwalch When this gets merged, it'd be great if a patch release can be made 😊.

Thank you very much again!

package.json Outdated
@@ -11,7 +11,7 @@
"bugs": {
"url": "https://github.com/video-dev/hls.js/issues"
},
"main": "./dist/hls.js",
"browser": "./dist/hls.js",

Choose a reason for hiding this comment

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

Why?

Besides, maybe this should go into another PR so we have this isolated in case of issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reversed

Copy link

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀🚀

@robwalch robwalch force-pushed the feature/named-exports branch from b3dd92b to ec88a98 Compare November 3, 2023 21:29
@robwalch robwalch enabled auto-merge (rebase) November 3, 2023 21:35
@ferferga
Copy link

ferferga commented Nov 6, 2023

@robwalch I believe the failing test is completely unrelated to this? Maybe a rebase against master could fix it?

@robwalch robwalch merged commit 0b20626 into master Nov 6, 2023
16 of 17 checks passed
@robwalch robwalch deleted the feature/named-exports branch November 6, 2023 16:15
@robwalch
Copy link
Collaborator Author

robwalch commented Nov 6, 2023

@robwalch I believe the failing test is completely unrelated to this? Maybe a rebase against master could fix it?

The test failure is a known issue in older versions of Safari.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enums only exported as types
3 participants