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(web): removed dynamic shaka-player import #2292

Closed
wants to merge 3 commits into from

Conversation

Bram-dc
Copy link
Contributor

@Bram-dc Bram-dc commented Apr 6, 2024

Let bundlers decide how to import the package instead of forcing a dynamic import.

@Bram-dc Bram-dc requested a review from dcvz as a code owner April 6, 2024 16:58
@Bram-dc Bram-dc changed the title Removed dynamic shaka-player import on web fix(web): removed dynamic shaka-player import Apr 6, 2024
web/TrackPlayer/PlaylistPlayer.ts Outdated Show resolved Hide resolved
web/TrackPlayer/Player.ts Show resolved Hide resolved
added types for shaka-player
@Bram-dc
Copy link
Contributor Author

Bram-dc commented Apr 18, 2024

I now finally understand the issue. Not sure how to solve it :(

@jspizziri
Copy link
Collaborator

@Bram-dc , can you describe the issue you're having with the dynamic import?

@Bram-dc
Copy link
Contributor Author

Bram-dc commented Apr 18, 2024

Yeah, I will create a sample project!

@Bram-dc
Copy link
Contributor Author

Bram-dc commented Apr 18, 2024

@jspizziri https://github.com/Bram-dc/rntp-web-demo

shaka-player will be undefined.

It is an easy fix by forcing the bundler to transform the shaka file again. See the commented lines in vite.config.mts. I was about to close this PR, because of that. It would be nice however if we would not need to do this extra step.

@Bram-dc
Copy link
Contributor Author

Bram-dc commented Apr 18, 2024

I also think this has become an issue because shaka-player is distributed for these modern javascript projects and not a "bug" in RNTP. By importing the file directly bundlers will almost in every case do this extra step automatically. The dynamic import will break the code in specific bundlers. In my projects I use vite as bundler and it has an easy fix for this problem. Maybe other bundlers do not have this options. This problem affects only a very small group of people (react-native-web + rntp + modules), and not worth patching in this library. (shaka-player is the issue because it is not made for these environments).

@jspizziri
Copy link
Collaborator

@Bram-dc this is interesting... it seems like Vite is doing something wrong. Dynamic imports are part of the JS spec so I'm not sure why it's not doing it properly. Do you have any insights there? (never used vite and I'm old enough to have bundler fatigue at this point haha).

When I ran your project, I did notice that something from shaka was importing (specifically shaka.default existed), but I'm not sure why things are breaking. I'll have to take a bit more time digging into it. But generally speaking, something doesn't smell right to me at the moment.

jspizziri added a commit to 5-stones/react-native-track-player that referenced this pull request Apr 19, 2024
@jspizziri
Copy link
Collaborator

@Bram-dc please take a look at this PR

#2299

@jspizziri
Copy link
Collaborator

I'm going to close this PR in favor of the one I just opened. In short, I don't think dynamic imports in and of themselves are the issue here.

@jspizziri jspizziri closed this Apr 19, 2024
@jspizziri jspizziri linked an issue Nov 7, 2024 that may be closed by this pull request
jspizziri added a commit to 5-stones/react-native-track-player that referenced this pull request Nov 7, 2024
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.

2 participants