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

Error on Destroying Splide Instance with Unvisited HTML Video Slide #58

Open
2 tasks done
Elwood-P opened this issue Oct 11, 2024 · 0 comments
Open
2 tasks done
Labels
bug Something isn't working

Comments

@Elwood-P
Copy link

Elwood-P commented Oct 11, 2024

Checks

Version

0.8.0

Description

When using the Splide extension for video, an error occurs if you attempt to destroy a Splide instance containing an HTML video slide that hasn't been visited. I think the error is due to the destroy method in the HTMLVideoPlayer class not checking if the player exists before attempting to destroy it. I think the method also incorrectly uses addEventListener instead of removeEventListener.

  1. Create a Splide instance with multiple slides, including at least one slide containing an HTML video.
  2. Do not navigate to the slide with the video.
  3. Attempt to destroy the Splide instance.

Proposed Solution:

Add check for existence of player.
Replace addEventListener with removeEventListener.

Current code:
https://github.com/Splidejs/splide-extension-video/blob/e71a80264d9e384caf3ddca54227b339cc75e660/src/js/players/html/HTMLVideoPlayer.ts

 destroy(): void {
    super.destroy();

    const { player } = this;
    const off = player.addEventListener.bind( player );

    off( 'play', this.onPlay );
    off( 'pause', this.onPause );
    off( 'ended', this.onEnded );
    off( 'loadeddata', this.onPlayerReady );
  }

Suggested amendment:

 destroy(): void {
    super.destroy();

    const { player } = this;
    if (!player) return;

    const off = player.removeEventListener.bind( player );

    off( 'play', this.onPlay );
    off( 'pause', this.onPause );
    off( 'ended', this.onEnded );
    off( 'loadeddata', this.onPlayerReady );
  }

Reproduction Link

https://codepen.io/ElwoodP/pen/mdNRWpO

Steps to Reproduce

  1. Without navigating to slide 3 (the video slide), click the "Destroy" button.
  2. The Splide instance will not be destroyed and a console error will be produced:
splide-extension-video.esm.js:552 Uncaught TypeError: Cannot read properties of undefined (reading 'addEventListener')
    at ae.destroy (splide-extension-video.esm.js:552:24)
    at fe.destroy (splide-extension-video.esm.js:3440:19)
    at splide-extension-video.esm.js:3472:14
    at O (splide-extension-video.esm.js:272:13)
    at Object.u [as destroy] (splide-extension-video.esm.js:3471:5)
    at splide.esm.js:3118:40
    at splide.esm.js:139:30
    at Array.forEach (<anonymous>)
    at it (splide.esm.js:138:59)
    at n.destroy (splide.esm.js:3117:7)

Expected Behaviour

Splide instance should be destroyed without errors.
A "Destroyed." message should appear in the console log showing the instance destroy event has been successfully triggered.

@Elwood-P Elwood-P added the bug Something isn't working label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant