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

Warnings with React 18 #1439

Closed
dhruvin25799 opened this issue Apr 17, 2022 · 12 comments
Closed

Warnings with React 18 #1439

dhruvin25799 opened this issue Apr 17, 2022 · 12 comments

Comments

@dhruvin25799
Copy link

dhruvin25799 commented Apr 17, 2022

Current Behavior

Getting infinite warnings could not call getDuration, getCurrentTime, getVideoLoadedFraction on React 18.
This is only happening when React 18 with createRoot is used.
The app works fine, the video works fine only these infinite warnings are the issue.

image

Expected Behavior

Shouldn't have any warnings or any function calls.

Steps to Reproduce

  1. Codesandbox Link : https://codesandbox.io/s/snowy-field-u9fugp?file=/src/App.js

Environment

  • React 18 with createRoot
@cookpete
Copy link
Owner

It looks like some change in React 18 is messing up the callPlayer util. If I console.log(this.player) in the util I get this output:

image

Notice the target iframe changes from widget2 (correct) to widget4 (which doesn’t exist).

Looking back, the implementation of callPlayer is strange and probably very brittle.

@cookpete
Copy link
Owner

Looks like this issue also stops happening if you remove <StrictMode>

@juanfraherrero
Copy link

juanfraherrero commented Apr 23, 2022

Looks like this issue also stops happening if you remove <StrictMode>

Sorry, from where i should remove <StrictMode>?

@wimpykatana
Copy link

wimpykatana commented Apr 24, 2022

Looks like this issue also stops happening if you remove <StrictMode>

Sorry, from where i should remove <StrictMode>?

You can find it inside index.js

@cookpete
Copy link
Owner

cookpete commented May 2, 2022

Looks like this is fixed in React 18.1

@cookpete cookpete closed this as completed May 2, 2022
@chochihim
Copy link

@cookpete I am using React 18.1 but still getting these infinite warnings. FYI I am also using Next.js 12.1.6

@cookpete
Copy link
Owner

cookpete commented May 3, 2022

Yep you’re right, sorry. I was confused with a different issue.

@cookpete cookpete reopened this May 3, 2022
manishsharma004 added a commit to manishsharma004/react-player that referenced this issue May 5, 2022
@manishsharma004
Copy link

There seems to be an issue with double loading of Youtube api, which overrides the Youtube.player value.

Only first YT.Player instance get the updates recieved from iframe messages for update apiInterface that adds the getDuration, getCurrentTime etc. methods. Stopping the multiple load for a url solves the issue. @cookpete added a PR which may or may not be an ideal solution to issue.

@lehnerpat
Copy link

lehnerpat commented May 7, 2022

As a temporary workaround, you can use the patch below to apply @manishsharma004's changes from #1450 to your locally-installed NPM package. (Note: This patch is not exactly the same as the PR itself, because it applies to the compiled files, not the source files!)

How to apply the fix manually / one-time:

  • Save the patchfile contents below to a file Player.js.patch (note: filename and location don't matter, but I'll use this as an example)
  • In your project, run patch -i Player.js.patch node_modules/react-player/lib/Player.js (if your Player.js.patch file is located somewhere else, use the right path here)

How to apply this fix with a postinstall script (useful if your project is built by a CI server or you collaborate with other people):

  • Save the patchfile contents below to a file Player.js.patch and check it into your repo
  • in your package.json, add a "postinstall" script:
    ". . .",
    "scripts": {
      ". . .",
      "postinstall": "patch -i Player.js.patch node_modules/react-player/lib/Player.js -N || true"
    },
    ". . .",
  • adjust the path of the patch file as needed
  • Note: the -N argument and the || true make sure that the postinstall script doesn't fail if the patch is already applied (i.e. if npm install/yarn install is being run when react-player is already installed)
  • Be sure to remove this again when a fixed version of react-player is released and you update to it :)

--- node_modules/react-player/lib/Player.js	2022-05-07 18:04:18.000000000 +0200
+++ node_modules/react-player/lib/Player.js	2022-05-07 18:04:01.000000000 +0200
@@ -82,7 +82,10 @@
     _defineProperty(_assertThisInitialized(_this), "handlePlayerMount", function (player) {
       _this.player = player;

-      _this.player.load(_this.props.url);
+      if (!_this.loaded?.[_this.props.url]) {
+        _this.player.load(_this.props.url);
+        _this.loaded = { [_this.props.url]: true };
+      }

       _this.progress();
     });
@@ -288,7 +291,10 @@
         this.isLoading = true;
         this.startOnPlay = true;
         this.onDurationCalled = false;
-        this.player.load(url, this.isReady);
+        if (!this.loaded?.[url]) {
+          this.player.load(url, this.isReady)
+          this.loaded = { [url]: true }
+        }
       }

       if (!prevProps.playing && playing && !this.isPlaying) {

@manishsharma004
Copy link

Thanks @lehnerpat for the detailed guide on patch, however I wouldn't recommend patching your production code with a changes from someone who isn't a developer on the project(me) until it gets reviewed by project developers. So, I would suggest patching only if there's no other way.

@lehnerpat
Copy link

lehnerpat commented May 7, 2022 via email

@cookpete
Copy link
Owner

cookpete commented May 7, 2022

This should now be fixed, as of 2.10.1

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 a pull request may close this issue.

7 participants