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

Use Redux Toolkit instead of plain Redux #94

Open
wants to merge 2 commits into
base: v3
Choose a base branch
from

Conversation

DylanVann
Copy link

@DylanVann DylanVann commented May 11, 2020

  • 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?

@ibc
Copy link
Member

ibc commented May 11, 2020

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

@DylanVann
Copy link
Author

Sounds great, thank you!

@ibc
Copy link
Member

ibc commented May 14, 2020

Why is app/lib/utils.js removed?

@DylanVann
Copy link
Author

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.

@samames
Copy link

samames commented Mar 12, 2021

can we not use the context API?

@ibc
Copy link
Member

ibc commented Mar 12, 2021

can we not use the context API?

Yes, we can.

@samames
Copy link

samames commented Mar 12, 2021

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...

@ibc
Copy link
Member

ibc commented Mar 13, 2021

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.

@samames
Copy link

samames commented Mar 13, 2021

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.

@ibc
Copy link
Member

ibc commented Mar 13, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants