[Rewrite] General Improvements

[ash0x0]

I'm making a few general improvements to use this project. This is a PR to get discussion about these going and track progress.

The issues I mean to address are:

1. Removing development and testing code, specifically the server-side responsibility code and the default parameters for localhost testing. This doesn't belong here, perhaps it can be offered in a component specifically meant for testing or moving the testing parameters and code to test runner.
2. The lack of layout, UI and functionality customization. It's very difficult to customize the UI or the component layouts and for the most part you're left stuck with little to no ability to control the functions offered by the component. This is unusual for react components and doesn't match the general expectations of composition.
3. Rewrite to function components. In terms of lifecycle and management, these are much better to work with and have less issues, footprint and ambiguity.
4. Improve documentation, there is some documentation on the openvidu website and in the tutorial repo but this should have its user own documentation here.

@pabloFuente if I'm not mistaken, you're the one maintaining this repo for the moment. What do you think about these changes and are they generally welcome? I also have a question about the structure of this repo. Is there a reason this is split to the library and openvidu-call-react packages? As far as I can tell, it's all placed into library at the end of the day and published there and there doesn't seem to be any specific reason why openvidu-call-react is separated. Is this for some legacy compatibility reasons? If it'd be possible, I think it'd be better to change the structure to a single package under root, it would be easier to work with and contribute to, the structure right now is a little unusual. If there's a reason for this perhaps we can add to README.

[micaelgallego]

Hello @ash0x0,

The OpenVidu team is not expert in React.

We welcome your contributions to the OpenVidu-Call-React application.

[ash0x0]

Great. This now delivers all previous functionality with better verbosity, validation, customization options, slightly better performance for some components, and improvements to the repo organization and code.

Outstanding items are:

1. Testing all functionality and ensuring everything works. I tested what I know about but if someone's around who knows everything this is supposed to do, it'd be great if they'd test it and confirm.
2. Complete a rewrite of VideoRoomComponent. It currently produces a lifecycle-related error (has no side effects but an error nonetheless), a rewrite will resolve this.
3. Resolve styling issues related to material-ui. Also the rigidity of the layout and its lack of customization.
4. Update usage docs once everything else is taken care of.

Right now, this remains in draft state. It currently needs someone to review and decide if the decisions made here are acceptable. Also to test the functionality and make sure everything required is there.

[pabloFuente]

Hi,

I am not 100% sure, but I think the library and openvidu-call-react folders have a purpose. Second one is the application itself (https://docs.openvidu.io/en/latest/demos/openvidu-call-react/), an adaptation to react of our Angular openvidu-call application. First one is probably needed to build the openvidu-react NPM package (https://www.npmjs.com/package/openvidu-react), which is just a way of adding the app as a dependency to any react project (https://docs.openvidu.io/en/latest/tutorials/openvidu-library-react/).

[ash0x0]

Well, with these changes, both of these usecases are supported. We would just need to update the demo page to remove cd openvidu-call-react/openvidu-call-react from step 4 and everything would still work fine, exactly the same as before.

The reason I made the change is that it was weird and difficult to work with the old structure, also the tooling wasn't working well and there's really no technical reason why we should separate the two.

The solution I have has a few build steps to manipulate some files to build the library but usage-wise, this should work exactly the same as the old structure, the only noticeable changes are the build script names.

If this structure would be an issue for other parts of the project, we can introduce a link or a copy script to add back the old path while having the code reside at the root, similar to how library is right now.

[ash0x0]

@pabloFuente @micaelgallego Is the code related to screen sharing extension still needed? given that the extension is deprecated I'm wondering why it's here. Can it be removed?

[micaelgallego]

Yes, you can remove code related with screen share extension as it is no longer needed.

[ash0x0]

Ok not everything is done with this yet but it's working well enough to review. The only missing item is that sending messages in chat doesn't display a user image. The old implementation had the local user image not the remote (sending) user image though so it wasn't correct either way. To fix this and make it more like openvidu-call I'll make another PR to add the room entry screen where the user takes a picture of themselves. Another outstanding item that isn't yet fixed is refactoring VideoRoomComponent. This will be a major undertaking as the JQuery layout manager doesn't work with react and in order to refactor this component I'll need to refactor all component layouts and remove all the findbyId which will take even more time than this PR took.

Now though, I'll need some reviews on this to address anything that requires changing and merge it to move forward. I'll introduce a separate PR soon for the layout refactor based on this tree.

Let me know if you'd like me to squash commits.

[CSantosM]

Hello @ash0x0 ,

First of all, thank you for your contribution to OpenVidu, as you know we are not React developers and we need to the community to be up to date with all frameworks and web technologies that we support.

Secondable, I have check and test your updates and looks great, however I have come across some errors. I will detail them.

1. When React user receive a chat message, the console log show this error

2. The chat doesn't store messages, i mean, the React user only can see the last message received or sent. The messages are always overwritten

3. In a session with two users (OpenVidu Call Angular deployed on demos.openvidu.io and React, served locally) when the Angular user share the screen, the React user can see it perfectly. However, as soon as the Angular user mutes the camera, the React user unpublish the angular screen stream and can't see the webcam stream because it is muted. After this, the publish and unpublish logic is realy weird. I would bet for a wrong deletion policy on the streamDestroyed event in the React side. Checking the React app without the refactoring, works as expected.

4. React library: I have build and pack the library successfully, nevertheless it crashes when try running the openvidu-library-react tutorial because of some libraries are missing. As you know, openvidu-library-react needs to work fine with this library besides have to support the tutorial extra features posted in our docs.

We need these bug fixes to be able to merge your PR.

Regards

[ash0x0]

Thanks for the review, I'll get on these and update.

On the last point, this is actually intentional and I'll PR doc updates for openvidu-insecure-react, openvidu-library-react and openvidu-call-react once this PR is ready before we merge it.

The current way to use the library allows for this:

<OpvSession
  id="opv-session"
  sessionName={mySessionId}
  user={myUserName}
  openviduServerUrl={'https://localhost:4443'}
  openviduSecret={'MY_SECRET'}
  joinSession={this.handlerJoinSessionEvent}
  leaveSession={this.handlerLeaveSessionEvent}
  error={this.handlerErrorEvent}
/>

which provides the library component with the openvidu server url and secret and leaves the token-grabbing logic to the library. This shouldn't be the case. The openvidu address and secret shouldn't be in a frontend react application at all. As the secure tutorials indicate, this logic should be in the server-side. That's why I'm removing it from the library. Now the usage should be like this:

<OpvSession
  id="opv-session"
  sessionName={mySessionId}
  user={myUserName}
  token={openViduSessionToken}
  joinSession={this.handlerJoinSessionEvent}
  leaveSession={this.handlerLeaveSessionEvent}
  error={this.handlerErrorEvent}
/>

with openViduSessionToken grabbed from a backend api instead of the frontend and provided to the library components. openvidu-call-react will retain the token-grabbing logic for demo. openvidu-react library itself will not. I think that should be the correct behavior because the default usage for the library in the docs introduces insecurity in any application that uses openvidu-react so it should be removed altogether and delegated to the library user to get their own token, which I'll provide a guide for in the doc update.