mediasoup-demo icon indicating copy to clipboard operation
mediasoup-demo copied to clipboard

Use Redux Toolkit instead of plain Redux

Open DylanVann opened this issue 4 years ago • 10 comments

  • Redux toolkit is intended to reduce boilerplate.
  • Redux toolkit supports TypeScript better than plain Redux.
  • We could conditionally add redux-logger some other way if that is desired, in this I removed it. Alternatively you could use the Redux devtools.
  • The Redux devtools will work after this.
  • Redux toolkit adds redux-thunk by default.

  • ~~This also removes unused utils.~~ Separate PR now.
  • ~~In development this currently logs a warning about non-serializable values being put in the store. Something could be done about this in another PR, or I could resolve it in this PR.~~ I disabled this functionality for now.

As an aside from this PR:

I'm attempting to use mediasoup on a project I'm working on. I have got it working locally with this demo so the first hurdle is done. I have a lot of experience with frontend development generally, but not much with WebRTC or mediasoup. I think I could learn a lot by improving this demo if you'd be interested in PRs for that.

Some ideas:

  • Use Yarn workspaces so this repo only requires one yarn install, possibly only one yarn start to get things working.
  • Use Create React App so this doesn't have to maintain any toolchain stuff.
  • Use TypeScript (easy after you're using Create React App).
  • Use Redux Toolkit for action creation (should be done after TypeScript).
  • Add to the instructions a bit to help out people having trouble configuring things and getting this working locally.

Would you be interested in PRs for these things?

DylanVann avatar May 11 '20 05:05 DylanVann

Thanks for this. Please let me come back to this tomorrow or maybe tonight if I get some time.

ibc avatar May 11 '20 18:05 ibc

Sounds great, thank you!

DylanVann avatar May 11 '20 21:05 DylanVann

Why is app/lib/utils.js removed?

ibc avatar May 14 '20 07:05 ibc

It's not used anywhere in the code. Actually mediasoup-demo-app-media-query-detector in the HTML file is not used anywhere either.

This could be another PR. I'll split this.

DylanVann avatar May 16 '20 17:05 DylanVann

can we not use the context API?

samames avatar Mar 12 '21 13:03 samames

can we not use the context API?

Yes, we can.

ibc avatar Mar 12 '21 13:03 ibc

It would be great to see it updated to use functional components and the context API (and up-to-date packages). Possibly simplified a bit, too, if at all possible.

The documentation doesn't really provide any practical instructions for communication between client and server, and what needs to happen and in what order, so people will be working from the demo. It should be a simple create-react-app project, really, maybe use context if we really need global state...

samames avatar Mar 12 '21 23:03 samames

The documentation does provide specific information to communicate client and server.

We are not gonna refactor the demo app. We don't care about users that just want to check the demo app instead of reading the doc, honestly. And I don't want to work for free for them.

ibc avatar Mar 13 '21 00:03 ibc

Yeah, you're right, I must've missed that part. A lot of people learn by playing around with things, rather than reading literature - Why do you not care about them?

Fair enough that you don't want to work for free, I guessed it was a sponsored project.

samames avatar Mar 13 '21 00:03 samames

We already wrote docs. Don't want to work twice just because some people prefer to play with demos.

And no, nobody is gonna pay us for writing a better demo app.

ibc avatar Mar 13 '21 00:03 ibc