doublesymmetry / react-native-track-player

A fully fledged audio module created for music apps. Provides audio playback, external media controls, background mode and more!
https://rntp.dev/
Apache License 2.0
3.18k stars 980 forks source link

fix(web): fix issues with certain bundlers only containing `default` … #2299

Open jspizziri opened 2 months ago

jspizziri commented 2 months ago

…on the imported shaka module

https://github.com/doublesymmetry/react-native-track-player/pull/2292

jspizziri commented 2 months ago

@Bram-dc , can you take a look at this PR and see if it solves your issue?

jspizziri commented 2 months ago

@Bram-dc , did you get a chance to test this?

andordavoti commented 2 months ago

I'm on vacation for three weeks, but I'll test it with Expo as soon as I get back:)

Bram-dc commented 2 months ago

No I have not, I can do tomorrow.

Bram-dc commented 2 months ago

No, it does not unfortunately solve the issue. Maybe I should open a PR at shaka player to also create an esm build.

jspizziri commented 2 months ago

@Bram-dc it works in the demo app that you shared with me. Did you test it there (that's where I confirmed it)?

Bram-dc commented 2 months ago

Make sure to disable the caching of the vite bundler when testing. It probably saved an earlier "optimized" version, as vite calls it, of the package. "optimized" meaning it created a transformed version. Sometimes it does do it, sometimes not, but when it does not transform the shaka-player package errors occur.

Adding the following to the Vite config, forces Vite to always transform the package with ESbuild.

    optimizeDeps: {
        include: ['shaka-player/dist/shaka-player.ui'],
    },

Works fine with the current way we do it now, so I suggest we close this PR for now, and people that use this specific project structure can do it this way.

jspizziri commented 2 months ago

@Bram-dc it works for me on the first run from scratch (rm -rf node_modules/.vite). Are you sure you're testing it correctly? Please document the steps you're taking to try and run this patch.

Bram-dc commented 2 months ago

You can test it when the package is not cached using this:

    optimizeDeps: {
        exclude: ['shaka-player/dist/shaka-player.ui'],
    },
Bram-dc commented 2 months ago

It probably saved an earlier "optimized" version, as vite calls it, of the package.

It also does this when running the bundler without any include. It probably does it, but not always.

Bram-dc commented 2 months ago

https://vitejs.dev/guide/dep-pre-bundling

During development, Vite's dev serves all code as native ESM. Therefore, Vite must convert dependencies that are shipped as CommonJS or UMD into ESM first. When converting CommonJS dependencies, Vite performs smart import analysis so that named imports to CommonJS modules will work as expected

Maybe it is the dynamic import that confuses Vite. I am not really sure, and would have to check a later time.

jspizziri commented 2 months ago

@Bram-dc yea, its still working for me. https://github.com/jspizziri/rntp-web-demo/commit/5b0598d03634dfc59f6d7120d0c30d7fe29570f6

andordavoti commented 1 month ago

didn't seem to fix the issue for me neither, could we use require instead of the dynamic import here for now to avoid this issue?

jspizziri commented 1 month ago

@andordavoti no, as I've already mentioned, that would break other things. Also, have you tested and confirmed that a static require fixes the expo issue? Despite what @Bram-dc reported this fix does actually solve the issue for vite. So really the only remaining one would appear to be expo.

Edit: if you could send me an example expo repo where this isn't working that would be helpful.

andordavoti commented 3 weeks ago

Sorry for the late response @jspizziri. I've not tested with a static require, it's just an assumption, should've tested that before suggesting it, sorry about that.

I created a repro with a simple expo app. If you launch it on web and look in the console you will get the error "Error: You must call setupPlayer prior to interacting with the player.".

GitHub repro: https://github.com/andordavoti/RNTP-WEB/tree/2299-fix-expo-web