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

[Bug] CldVideoPlayer load error (Next 13) #279

Open
leebaduk opened this issue Sep 12, 2023 · 8 comments
Open

[Bug] CldVideoPlayer load error (Next 13) #279

leebaduk opened this issue Sep 12, 2023 · 8 comments
Assignees

Comments

@leebaduk
Copy link

Bug Report

Describe the bug

When rendering multiple CldVideoPlayers on one page, some of the players fail to load a video (while others work fine, which is odd).
For example when I render 20 players on 5*4 grid which share the same src, 3 or 4 of them would be shown as a white blank randomly.

Is this a regression?

not sure

Steps To Reproduce the error

  1. repo: https://github.com/leebaduk/next-cloudinary-test
  2. create .env on root with NEXT_PUBLIC_CLOUDINARY_CLOUD_NAME=dlz0shbp1 (this is my cloud name)
  3. npm i
  4. npm run dev

Expected behaviour

all the videos should be successfully loaded.

CodeSandbox or Live Example of Bug

codesandbox available here, just the same as the repo above;
https://codesandbox.io/p/sandbox/next-cloudinary-test-y5hssp

Screenshot or Video Recording

Screenshot 2023-09-12 at 3 36 54 PM Screenshot 2023-09-12 at 3 45 09 PM

Your environment

  • OS: MacOS Ventura
  • Node version: 18.16.0
  • Npm version: 9.5.1
  • Browser name and version: Chrome

Additional context

@colbyfayock
Copy link
Collaborator

hey @leebaduk thanks for reporting this. strange behavior here and havent pinned down the issue yet.

Are you noticing this when all the videos are different sources as well?

do you have a specific use case for this many instances of the same video?

@leebaduk
Copy link
Author

leebaduk commented Sep 19, 2023

@colbyfayock sorry i got back to you late
when the videos are of different sources, it does not happen.

but i use router.replace(query parameter) on my app to search the videos.
and when i go back and forth between two pages,
say, /search?query=1 to /search?query=2 and back to /search?query=1,
the cldvideoplayer on /search?query=1 is not re-rendered, and "sometimes" shows as blank.
the possibility of it shown as a blank increases as I go back and forth more times.

so, even though I don't have a specific case for many instances of the same video, it still matters practically.

@colbyfayock
Copy link
Collaborator

ah got it, yeah that makes sense, thanks for your patience on this bug, will try to get to it soon

@colbyfayock
Copy link
Collaborator

just adding i was able to reproduce this by simply playing the videos on the Basic Usage page, navigating away, playing videos on another page, and navigating back

@JoshuaRotimi
Copy link
Contributor

@colbyfayock I would like to work on this issue. Can you assign it to me ?

@colbyfayock
Copy link
Collaborator

yes @JoshuaRotimi all yours!

colbyfayock added a commit that referenced this issue Nov 9, 2023
…317)

# Description

This PR fixes the issue #279 where some players fail to load when
rendering multiple `CldVideoPlayer` on one page.
- Also fixes the issue where the `CldVideoPlayer` component fails to
render properly after navigating between pages
- Also adds a cleanup function that removes the widget instance when a
`CldUploadWidget` component unmount

## Issue Ticket Number
#279

Fixes #<ISSUE_NUMBER>

<!-- Specify above which issue this fixes by referencing the issue
number (`#<ISSUE_NUMBER>`) or issue URL. -->
<!-- Example: Fixes
https://github.com/colbyfayock/next-cloudinary/issues/<ISSUE_NUMBER> -->

## Type of change

<!-- Please select all options that are applicable. -->

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] Fix or improve the documentation
- [ ] This change requires a documentation update


# Checklist

<!-- These must all be followed and checked. -->

- [x] I have followed the contributing guidelines of this project as
mentioned in [CONTRIBUTING.md](/CONTRIBUTING.md)
- [x] I have created an
[issue](https://github.com/colbyfayock/next-cloudinary/issues) ticket
for this PR
- [x] I have checked to ensure there aren't other open [Pull
Requests](https://github.com/colbyfayock/next-cloudinary/pulls) for the
same update/change?
- [x] I have performed a self-review of my own code
- [x] I have run tests locally to ensure they all pass
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes needed to the documentation

---------

Co-authored-by: Colby Fayock <colbyfayock@users.noreply.github.com>
@colbyfayock
Copy link
Collaborator

@leebaduk the latest release should improve the reliability of this, but there's still an issue where the Next.js Script tag won't reliably fire on every single onLoad instance. trying to determine a way to resolve this, but in the meantime, hoping this greatly helps

github-actions bot pushed a commit that referenced this issue Nov 9, 2023
# [5.4.0](v5.3.0...v5.4.0) (2023-11-09)

### Features

* dispose of upload widget and video player instance on unmount ([#317](#317)) ([a313b7a](a313b7a)), closes [#279](#279) [#279](#279)
@colbyfayock
Copy link
Collaborator

if anyone would like ot help with this - can someone reproduce the issue of using the <Script onLoad feature to load an external script that's mounted inside of a component? the component can be inside of the project, as opposed to a dependency

the loading mechanism can be displayed using console logs

if this is reproducible outside of the context of this library, i can take it to the Next.js team or raise an Issue

cham74 added a commit to cham74/next-image-delivery that referenced this issue Dec 23, 2023
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

No branches or pull requests

3 participants