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

fix!: bundle stream-browserify/process by default #767

Merged
merged 1 commit into from
Sep 24, 2023
Merged

Conversation

steabert
Copy link
Member

@steabert steabert commented Sep 24, 2023

Ever since the change away from automatically bundling browser-equivalents of Buffer and stream Node.js dependencies, there have been a number of issues with bundling. Since this is such a problem, the proposal is to fix it by including the bundle as default, prioritizing ease-of-use over bundle size.

The bundle size in most cases won't be affected however, so this change should go mostly unnoticed except for people using their own bundler (vite, CRA, webpack, etc) having less headaches.

For those very special use-cases where it's adamant to bundle manually, there is a "light" version of media-stream-library where you need to deal with the bundling yourself.

Fixes #754

@steabert steabert requested a review from rikteg as a code owner September 24, 2023 05:49
@steabert steabert marked this pull request as draft September 24, 2023 05:55
@steabert
Copy link
Member Author

steabert commented Sep 24, 2023

Still checking some things, as the dependencies don't make much sense. If stream is not included in the bundling, the entire problem is actually shifted onto the bundler and the polyfill won't work anyway. It's probably not worth the effort to keep it external? The idea was to minimize total bundle size, but that would be for situations where someone is already using stream-browserify for another purpose not related to this library. This seems like an edge case.

Alternatively, we could bundle the default (i.e. heavy) and offer a light variant with nothing bundled or polyfilled, where you are on your own, for those specific cases. This would prioritize ease-of-use over advanced bundle size management. But seeing the large amount of issues come up after we stopped bundling all of this by default, I think it's probably worth it to flip priorities.

If using both player and streams it makes sense that the player has streams as external, since you don't want double that size, so there it does make sense.

EDIT: updated the PR accordingly.

@steabert steabert changed the title fix: custom polyfill process/browser fix!: bundle stream-browserify/process by default Sep 24, 2023
@steabert steabert marked this pull request as ready for review September 24, 2023 07:47
@steabert steabert requested a review from Tigge as a code owner September 24, 2023 07:47
For browsers, always bundle `stream` from `stream-browserify`,
`buffer` and `process`. Include polyfill for `process/browser`
using the custom global variable name `process_browser` and
set a global `Buffer`.

This should selectively polyfill it only for the `readable-stream`
dependency which we use, and not interfere with any other usage
of `process`.

BREAKING CHANGES:
 - The `heavy` variants are no longer in use, this is now the default.
   Replace `media-stream-library/heavy` with `media-stream-library`.
   Replace `media-stream-player/heavy` with `media-stream-player`.

NOTE:
 - The bundle size might grow if you bundle `stream`/`buffer`/`process`
   separately. Make sure both `media-stream-library` and `media-stream-player`
   are at the latest major version.
 - There is a `light` variant of the `media-stream-library` for very
   specific use cases. To use it, please refer to the documentation,
   but in general it means you are reponsible for dealing with any of
   the non-included depedencies.

Fixes #754
@steabert steabert merged commit c785057 into main Sep 24, 2023
5 checks passed
@steabert steabert deleted the process-polyfill branch September 24, 2023 08:16
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.

"process.env" gets set to empty object in Vite dev environment when importing media-stream-player
1 participant