toggleHowler: don't seek if no props.seek specified

[svlapin]

Hey guys,

thanks for the nice wrapper!

I think I found a corner case when toggleHowler is fired and underlying howler._sounds is empty (e.g. preload="false"):

Uncaught (in promise) TypeError: Cannot read property '_id' of undefined
    at Howl.seek (howler.js:1405)
    at ReactHowler.seek (ReactHowler.js:252)
    at ReactHowler.toggleHowler (ReactHowler.js:122)
    at ReactHowler.componentWillReceiveProps (ReactHowler.js:54)

As Howler itself doesn't expect to receive seek request when there is no underlying sound, I guess the wrapper should not try to seek on each update unless props.seek is set.

[Stenerson]

Hi @svlapin, good catch! I want to run a few tests on this then I’ll merge and publish an update. Should be in the next 24h or so.

[svlapin]

Great, @Stenerson, thanks ! And I've just realized - probably that's better to

if (typeof props.seek !== 'undefined' && props.seek !== this.seek()) {

to handle seek === 0 properly.
Please let me know if I should update the PR.

[thangngoc89]

I think I could make this a potential candidate for the e2e testing I started here #56

@svlapin I'm not clear how can I reproduce this ?

[svlapin]

@thangngoc89, the following diff applied to examples/react/src/App.js makes demo page fail to bootstrap:

diff --git a/examples/react/src/App.js b/examples/react/src/App.js
index a9c6ff3..695d743 100644
--- a/examples/react/src/App.js
+++ b/examples/react/src/App.js
@@ -3,6 +3,11 @@ import SourceLink from './components/SourceLink'
 import { OnlyPlayPauseButton, NoPreload, SwapSource, AutoPlay, FullControl } from './players'
 
 class App extends React.Component {
+  componentDidMount() {
+    // Make children receive props so #toggleHowler is called
+    this.setState({});
+  }
+
   render () {
     return (
       <div className='container'>

howler.js:1405 Uncaught TypeError: Cannot read property '_id' of undefined
    at Howl.seek (howler.js:1405)
    at ReactHowler.seek (ReactHowler.js:252)
    at ReactHowler.toggleHowler (ReactHowler.js:122)
    at ReactHowler.componentWillReceiveProps (ReactHowler.js:54)
    at callComponentWillReceiveProps (react-dom.development.js:9795)
    at updateClassInstance (react-dom.development.js:9974)
    at updateClassComponent (react-dom.development.js:10224)
    at beginWork (react-dom.development.js:10605)
    at performUnitOfWork (react-dom.development.js:12573)
    at workLoop (react-dom.development.js:12682)

The above error occurred in the <ReactHowler> component:
    in ReactHowler (at NoPreload.js:41)
    in div (at NoPreload.js:40)
    in NoPreload (at App.js:31)
    in section (at App.js:28)
    in div (at App.js:16)
    in div (at App.js:13)
    in App (at index.js:8)

Happens to unloaded sounds only, so works fine if NoPreload player is commented out for example.

[Stenerson]

I am able to reproduce this by adding a "seek" button to the "Preload Disabled" example that sets a seek parameter to anything other than 0 before the sound is loaded.

render() {
    return (
      <div>
        <ReactHowler
          preload={false}
          ref={ref => (this.player = ref)}
          src={["sound2.ogg", "sound2.mp3"]}
          playing={this.state.playing}
          seek={this.state.seek}
          onLoad={() => this.setState({ loadState: this.player.howlerState() })}
        />
        {this.state.loadState === "unloaded" && (
          <Button className="full" onClick={this.handleLoad}>
            Load (Optional)
          </Button>
        )}
        <br />
        <Button onClick={this.handlePlay}>Play</Button>
        <Button onClick={() => (this.setState({seek: 2}))}>Seek!</Button>
        <Button onClick={this.handlePause}>Pause</Button>
      </div>
    );

[Stenerson]

if (typeof props.seek !== 'undefined' && props.seek !== this.seek()) { Doesn't seem to be fixing it for me.

What if we checked the howler state before calling this.howler.seek() in the seek function?

  seek(pos = null) {
    if (!this.howler || this.howlerState() === 'unloaded') {
      return 0;
    }

    // ... etc.

[svlapin]

@Stenerson
just to clarify

I am able to reproduce this by adding a "seek" button to the "Preload Disabled" example that sets a seek parameter to anything other than 0 before the sound is loaded.

If that's on current master branch, then any setState will throw in NoPreload player (#57 (comment)).

Otherwise, you're right, this PR doesn't fix the case of real attempt to seek in unloaded sound.
This case throws internal Howler error, and IMO that not the wrapper's responsibility to handle it, so checking howlerState should be rather implemented in the Howler itself.

Anyway, I'd be fine with either solution 👍

[Stenerson]

Ahh, yeah. I see what you're saying... 🤔

Something about this felt familiar. See goldfire/howler.js#796 & goldfire/howler.js#797 as well as #41.

I initially misunderstood the problem that you were solving but now I see. Passing undefined in to howler's seek before a sound is loaded does indeed cause it to crash. It probably shouldn't (want to open a new PR in howler.js? 😉)

I think it's reasonable for us to "protect" against it though by not calling seek "accidentally." I like the second solution you came up with, to check if (typeof props.seek !== 'undefined' && props.seek !== this.seek()) {

Can you update to that method and I'll merge/publish? Thanks!

[svlapin]

Done

[Stenerson]

@thangngoc89, if you’re looking for an EASY way to reproduce this for e2e tests you can just push the existing pause button before the sound is loaded on the existing no preload example.

Before @svlapin’s changes - crash
After - 👍

[Stenerson]

👍