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

no sound when src changes before howlerjs loads asynchronously #85

Open
dan-2019 opened this issue Jul 1, 2021 · 5 comments
Open

no sound when src changes before howlerjs loads asynchronously #85

dan-2019 opened this issue Jul 1, 2021 · 5 comments

Comments

@dan-2019
Copy link

dan-2019 commented Jul 1, 2021

My setup:

const [sound, setSound] = useState(null)
const [play] = useSound(sound, ... )

in the useEffect hook I then fetch the sound-url and once fetched set it with setSound(soundSrc)
In my case if the fetch happens "too soon", no sound is played.

The culprit is the lazy loading of howler. As it loads the useEffect hook that is triggered on sound source change runs. The test that ensures the howler component is initialized fails and no further action is taken. Sound source remains unchanged, in my case, as it was null no sound is played.

The solution is trivial, add the howler loading state into the dependency array of the useEffect hook:

React__default.useEffect(function () {
...
}, [JSON.stringify(src), isMounted.current]);

This ensures the hook re-runs once howler is loaded. Now, of course, the hook will also be called every time howler finishes loading, maybe a test should be added to avoid the extra needless initialization of howler inside the hook? Regardless, for me this change alone solved the issue.

@ZenBerry
Copy link

Thank you so much for sharing this!

I've got the same issue, but your suggestion didn't work for me for some reason.
Do I understand correctly that I've got to add isMounted.current to the dependency of the useEffect hook in the useSound library?

I've modified use-sound.esm.js and use-sound.cjs.development.js but nothing happens.

It would be very nice of you if you could please suggest the correct solution.

Have a lovely day!
Best, Eugene

@dan-2019
Copy link
Author

Eugene,

As I understand it, you cannot directly modify a node package. My hope was (is) that the package's author change the source code and republish it on npm.

In the meantime (if and until the author makes the changes) you can use the patch-package (https://www.npmjs.com/package/patch-package) to do the patching.

Hope this helps,
Dan

My patch file:

diff --git a/node_modules/use-sound/dist/use-sound.cjs.development.js b/node_modules/use-sound/dist/use-sound.cjs.development.js
index c6b1bdc..d2e7ba7 100644
--- a/node_modules/use-sound/dist/use-sound.cjs.development.js
+++ b/node_modules/use-sound/dist/use-sound.cjs.development.js
@@ -146,7 +146,7 @@ function useSound(src, _ref) {
     // https://github.com/facebook/react/issues/14476#issuecomment-471199055
     // eslint-disable-next-line react-hooks/exhaustive-deps
 
-  }, [JSON.stringify(src)]); // Whenever volume/playbackRate are changed, change those properties
+  }, [JSON.stringify(src), isMounted.current]); // Whenever volume/playbackRate are changed, change those properties
   // on the sound instance.
 
   React__default.useEffect(function () {
diff --git a/node_modules/use-sound/dist/use-sound.esm.js b/node_modules/use-sound/dist/use-sound.esm.js
index 6ec2b77..0dea43b 100644
--- a/node_modules/use-sound/dist/use-sound.esm.js
+++ b/node_modules/use-sound/dist/use-sound.esm.js
@@ -120,7 +120,7 @@ function useSound(src, _ref) {
     // https://github.com/facebook/react/issues/14476#issuecomment-471199055
     // eslint-disable-next-line react-hooks/exhaustive-deps
 
-  }, [JSON.stringify(src)]); // Whenever volume/playbackRate are changed, change those properties
+  }, [JSON.stringify(src), isMounted.current]); // Whenever volume/playbackRate are changed, change those properties
   // on the sound instance.
 
   React__default.useEffect(function () {

@joshwcomeau
Copy link
Owner

Ah, yeah I hadn't really expected this to be used with dynamic / changing sound URLs.

The trouble with the proposed fix is that isMounted is a ref, and ref changes don't trigger a re-render. Meaning that this solution will only work if the component happens to re-render after the fact for another reason. I suspect that your fix either isn't working 100% of the time, or if it is, only because there's some consistent reason that the component is rendering (which won't be true in other usecases). At least, that's my initial impression.

I think the more-idiomatic fix would be to catch the specific case where the src changes before Howler is loaded, and schedule an update that sets it immediately when Howler loads... but that would complicate things quite a bit, and I'm not sure I want to go down that road. I've been pretty aggressive about keeping the codebase simple, even at the expense of certain usecases.

Honestly the simplest thing might be to fork this project and load Howler directly (not through a lazy-load). That's not a trade I want to make in this package, but it would be a guaranteed way to solve this problem without any additional code complexity.

@squalvj
Copy link

squalvj commented Jan 27, 2022

+1

1 similar comment
@pavlyutkin
Copy link

+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

No branches or pull requests

5 participants