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

[jspizziri]

…on the imported shaka module

#2292

[jspizziri]

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

[jspizziri]

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

[andordavoti]

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

[Bram-dc]

No I have not, I can do tomorrow.

[Bram-dc]

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

[jspizziri]

@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]

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]

@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]

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

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

[Bram-dc]

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]

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]

@Bram-dc yea, its still working for me. jspizziri/rntp-web-demo@5b0598d

[andordavoti]

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]

@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]

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

[github-actions]

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[YajanaRao]

I am facing the same issue on web:

Importing the player before setupPlayer method fixes the issue for now

const shaka = require("shaka-player/dist/shaka-player.ui");

[jspizziri]

@puckey I think if no one confirms or rejects this PR in the next week we should just merge it. It seems to be working in my testing, and no one has submitted any repro's allowing me to test against their reported issue.

[andordavoti]

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

@jspizziri Have you looked at this repro?

[jspizziri]

@andordavoti sorry somehow I missed that one. I'll take a look at it today.

[jspizziri]

@andordavoti after investigation, it appears that Expo (circa March 29 of this year) does not support dynamic/async imports (although new versions of the metro bundle do).

RNTP Web uses dynamic imports in order to support SSR (e.g. on Next.js).

The guidance provided on that ticket is to:

1. Install @expo/metro-runtime.
2. import @expo/metro-runtime in your root app file:

// App.tsx
import '@expo/metro-runtime';
...

I think what's needed here is to add some documentation for Expo Web users to alert them of this and add documentation about either importing metro-runtime or manually requiring shaka-player. Here's a PR with the changes that fix your issues:

andordavoti/RNTP-WEB#1

Edit: I also just checked expo v51 and canary and the issue is there too.

[andordavoti]

Thanks for getting back to me this quick with this. Could you create a PR where this is added to the docs?

[andordavoti]

Thanks man, really appreciate all the work you have done on this, this is awesome!!

[jspizziri]

@dcvz I think this is ready to be merged. The failing builds are due to iOS and unrelated.