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 React hook to better deal with websockets #3299

Merged
merged 2 commits into from
May 1, 2024
Merged

Conversation

ramondeklein
Copy link
Collaborator

@ramondeklein ramondeklein commented Apr 18, 2024

The initial goal with this PR is to avoid that starting the tracing could be done more often (see #3283). It would have been possible to keep a boolean flag that the websocket was being opened. I think it would be more idiomatic to deal with this using React's built-in mechanisms.

It now uses the logActive state to determine whether the web-socket should be connected. It uses the wsUrl state to determine the actual URL. The URL is updated accordingly to the individual flags and filters. This would also allow to dynamically switch to another URL if the flags/filters are updated. This is now only blocked, because the HTML elements are disabled when tracing is active.

Using a web-socket in React works much better with the useWebSocket component. It also provides a built-in ping mechanism and supports typed responses (no need to unmarshal JSON in our code).

PS: Due to the change it's probably better to compare this side-by-side instead of unified diffing.

@ramondeklein ramondeklein marked this pull request as draft April 18, 2024 13:20
@ramondeklein ramondeklein linked an issue Apr 18, 2024 that may be closed by this pull request
@ramondeklein ramondeklein self-assigned this Apr 18, 2024
@ramondeklein ramondeklein force-pushed the fix-websocket branch 2 times, most recently from 2f96f07 to e218922 Compare April 18, 2024 13:32
@ramondeklein
Copy link
Collaborator Author

@cesnietor @bexsoft @dvaldivia I could make these changes for other places where we use websocket too, but first I want to know if you agree with this solution...

@bayasdev
Copy link
Contributor

@ramondeklein I like that the code looks more cleaner with the hook. I'm digging through the react-use-websocket source code and it seems to be using the native WebSocket API provided by JavaScript which is fine.

However, I still have some doubts about how we're going to integrate it in the rest of the application, specifically the object browser which currently uses a Redux middleware to update the local state with the WebSocket.

https://github.com/minio/console/blob/master/web-app/src/websockets/objectBrowserWSMiddleware.ts

@ramondeklein
Copy link
Collaborator Author

ramondeklein commented Apr 18, 2024

@bayasdev I haven't looked at the object browser yet. We may want to use the common code if that would fit better for that case. Not sure what middleware adds for an object browser. I typically use middleware to add authentication, logging, ... But I haven't taken a serious look.

react-use-websocket is a well-maintained and commonly used package. TBH, the use of server-side events would have been more appropriate here, but that would break the API.

I think the solution in the PR beats a naive fix that uses a boolean that disables the "start" button when it's clicked.

@ramondeklein ramondeklein marked this pull request as ready for review April 18, 2024 14:02
Copy link
Member

@prakashsvmx prakashsvmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested. LGTM 👍

@harshavardhana harshavardhana merged commit 02a0db1 into master May 1, 2024
29 checks passed
@harshavardhana harshavardhana deleted the fix-websocket branch May 1, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web Trace tool cannot stop streaming data through WebSocket
5 participants