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

Web-Viewer: Don't auto-connect to wss://hostname when an ?url= is missing #3345

Merged
merged 6 commits into from
Sep 18, 2023

Conversation

jprochazk
Copy link
Member

@jprochazk jprochazk commented Sep 18, 2023

@jprochazk jprochazk added the 🕸️ web regarding running the viewer in a browser label Sep 18, 2023
crates/re_viewer/src/web.rs Outdated Show resolved Hide resolved
web_viewer/index.html Outdated Show resolved Hide resolved
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

I'm not sure this is 100% the right approach.

The fallback_to_websockets: bool seems to me quite a hack, especially as it is controlled from JavaScript. It is, as far as I can tell, trying to subvert the fallback in get_url for what URL to connect to. Do we need this fallback at all though? All the places where we open the web-viewer for the user we already add the ?url=… query parameter for them.

For instance, cargo r -p raw_mesh -- --serve will print this:

Listening for websocket traffic on ws://localhost:9877. Connect with a Rerun Web Viewer.
Started web server on http://localhost:9090
Web server is running - view it at http://localhost:9090?url=ws://localhost:9877

and then it opens that last URL.

I think it would make more sense to remove the fallback from get_url and handle no URL by simply adding no receiver to the application. This will make the web-viewer without an url= query act similar to just running rerun, i.e. make it more similar to the native viewer

@jprochazk
Copy link
Member Author

I removed the fallback_to_ws path, and changed it to only add the receiver if url is present.

@jprochazk jprochazk requested a review from emilk September 18, 2023 11:44
@emilk
Copy link
Member

emilk commented Sep 18, 2023

Should we also remove this dropdown in this PR then?

image

The demo link in the PR description https://demo.rerun.io/pr/3345 does not show the welcome page

@jprochazk
Copy link
Member Author

jprochazk commented Sep 18, 2023

That's because demo.rerun.io gets the URL implicitly, like app.rerun.io did before this PR. I think getting rid of demo.rerun.io is a separate step, as is cleaning up all the related scripts and CI to not do redundant work anymore, setting up app.rerun.io/pr/<pr_number> so we can use that instead of demo.rerun.io, redirecting demo.rerun.io to app.rerun.io, etc.

@emilk emilk changed the title Don't fall back to websockets on app and demo Web-Viewer: Don't auto-connect to wss://hostname when an ?url= is missing Sep 18, 2023
crates/re_viewer/src/web.rs Show resolved Hide resolved
crates/re_viewer/src/web.rs Outdated Show resolved Hide resolved
@jprochazk jprochazk merged commit d96661c into main Sep 18, 2023
@jprochazk jprochazk deleted the jan/app-no-default-rrd branch September 18, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕸️ web regarding running the viewer in a browser
Projects
None yet
2 participants