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

Avoid re-exporting enums with "export type" #5685

Closed

Conversation

ericmatthys
Copy link

@ericmatthys ericmatthys commented Jul 20, 2023

This PR will...

Re-export enums with a standard "export" instead of "export type".

Why is this Pull Request needed?

When using "export type" with enums, the enums are not bundled in and cannot be use as values, even though they'll be included in type declaration files. This leads to runtime errors that satisfy the type checker. Reserve "export type" only for types and interfaces that cannot be used at runtime.

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

HdcpLevels is not an enum, but it is an array that can be used as a value at runtime.

Resolves issues:

Closes #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 previously approved these changes Jul 20, 2023
@robwalch robwalch added this to the 1.5.0 milestone Jul 20, 2023
@ericmatthys
Copy link
Author

I ran prettier to fix the lint error

Copy link
Collaborator

@robwalch robwalch left a comment

Choose a reason for hiding this comment

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

It looks like this would introduce a breaking change requiring named imports:

[!] Error: Entry module "src/hls.ts" is using named and default exports together. Consumers of your bundle will have to use `Hls.default` to access the default export, which may not be what you want. Use `output.exports: "named"` to disable this warning

This is likely why we add static getting to Hls for portions of the API we expect developers to use as runtime values (ex: Hls { get Events() } ).

@ericmatthys
Copy link
Author

Oof, yeah. Some potential options...

  • Don't use enums for the exported types to avoid the runtime error without a type error problem
  • Switch to all named exports (breaking change)
  • Switch to ESM only distribution (not sure if that's realistic)

@robwalch
Copy link
Collaborator

Don't use enums for the exported types to avoid the runtime error without a type error problem

Maybe the only viable option. I don't know if enums are providing any advantage size-wise in the ESM or minified ES5 output. I think we can get away with a compatible type change. Most important thing would be for them to remain consts.

@ericmatthys
Copy link
Author

I'll open up a separate PR if I can get something working with that approach.

@ericmatthys
Copy link
Author

#5729

@robwalch
Copy link
Collaborator

After reviewing #5729 I prefer looking into taking this PR, and providing an ESM build with named exports (minimal breaking change), and then somehow maintaining the default only export for the UMD build (no breaking change for UMD users)

#5729 (review)

@robwalch robwalch mentioned this pull request Oct 24, 2023
5 tasks
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
2 participants