hackingbeauty / react-mic

Record audio from a user's microphone and display a cool visualization.
https://hackingbeauty.github.io/react-mic/
452 stars 158 forks source link

onData still called after setting record=false #94

Closed clonq closed 4 years ago

clonq commented 4 years ago

onData seems to be triggered after "record" is set to "false", is that by design? The only difference between my code and the sample code is that I use hooks: const [recording, setRecording] = React.useState(false) Any hints?

hackingbeauty commented 4 years ago

Hey @clonq thanks for the heads up. This is a bug and not by design. I will patch shortly.

hackingbeauty commented 4 years ago

@clonq I just checked React-Mic, and onData stops when "record" is set to "false". Is there a problem in your code?

clonq commented 4 years ago

Maybe you see something that I don't:

import React from 'react';
import { ReactMic } from 'react-mic';
import { Button } from '@material-ui/core';

const Recorder = () => {
  const [recording, setRecording] = React.useState(false);
  const onStop = () => {
    console.log('done');
  }
  const onData = () => {
    console.log('this goes on ond on if not after the first click on stop, for sure after the second click on play and then on stop');
  }
  return <div>
    <ReactMic
      record={recording}
      onStop={onStop}
      onData={onData}
    />
    <Button variant="contained" onClick={e => setRecording(true)}>Start</Button>
    <Button variant="contained" onClick={e => setRecording(false)}>Stop</Button>
  </div>
}

export default Recorder
clonq commented 4 years ago

I think I found a workaround: don't use

clonq commented 4 years ago

If I wrap the component in I also get this warning:

Warning: A string ref, "visualizer", has been found within a strict mode tree. String refs are a source of potential bugs and should be avoided. We recommend using useRef() or createRef() instead. Learn more about using refs safely here: https://fb.me/react-strict-mode-string-ref

cephalization commented 4 years ago

@clonq did you workaround this by forking the lib? I don't think I am using strict mode in my repo with parcel, but I am encountering the same bug in a very similar piece of code to your own.

cephalization commented 4 years ago

@clonq did you workaround this by forking the lib? I don't think I am using strict mode in my repo with parcel, but I am encountering the same bug in a very similar piece of code to your own.

The workaround in my situation was to not conditionally render the mic based on recording bool but to hide it via css instead. When it unmounts and remounts it loses track of if it started recording or not so it does not call onStop.

mcapkovic commented 4 years ago

I copied the "demo" code to my app and I have the same problem as clonq (onData is not stopping even with "record" set to "false"). Project started with create-react-app is wrapped in by default and there are these warnings:

Warning: A string ref, "visualizer", has been found within a strict mode tree. String refs are a source of potential bugs and should be avoided. We recommend using useRef() or createRef() instead.

Warning: A string ref, "audioSource", has been found within a strict mode tree. String refs are a source of potential bugs and should be avoided. We recommend using useRef() or createRef() instead.

Like clonq wrote everything is working if I delete but the thing is, I would like to use StrictMode.

hackingbeauty commented 4 years ago

OK @clonq @mcapkovic @cephalization I made a fix and React-Mic should now work in .

Can someone update to version react-mic@12.4.2 and verify that the patch works so that I can close this issue?

Also, let me know what other changes you would like me to make (in separate tickets) so this component is more useful for you.

mcapkovic commented 4 years ago

@hackingbeauty it looks like react-mic@12.4.2 package contains only package.json, README and no js

hackingbeauty commented 4 years ago

@mcapkovic Sorry about that. Updated to react-mic@12.4.5 should work now :)

mcapkovic commented 4 years ago

@hackingbeauty no problem :) package is ok but the bug is still there. https://github.com/mcapkovic/react-mic/commit/b4fd537c09fe0893255fed902c41fea2ae522563 I recreated it in your example.

steps to reproduce:

hackingbeauty commented 4 years ago

@mcapkovic More fixes made. Can you check again? Please pull react-mic@12.4.6. Thanks!

mcapkovic commented 4 years ago

@hackingbeauty yes, everything is working with react-mic@12.4.6. thank you :)