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

feat: use request host for hot-reload instead of window.location.host #500

Closed
wants to merge 5 commits into from

Conversation

amrbashir
Copy link
Contributor

@amrbashir amrbashir commented Feb 22, 2023

Checklist

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md with pertinent info (may not always apply).
  • Updated site content with pertinent info (may not always apply).
  • Squash down commits to one or two logical commits which clearly describe the work you've done. If you don't, then Dodd will 🤓.

This is a continuation of #494, when using tauri on mobile, we proxy the requests and the files are served using a custom protocol https://tauri.localhost so window.location.host will be tauri.localhost and the hot-reload socket won't connect. To fix this, trunk will hard-code the address inside the auto-reload script.

@amrbashir amrbashir changed the title feat: use request uri for hot-reload instead of window.location.host feat: use request host for hot-reload instead of window.location.host Feb 22, 2023
Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Looks like this is going to need rebasing / updating. Also, while you are at it, do you mind elaborating a bit on what this solves and why this might be needed? Also, very importantly, I acknowledge that this may fix an issue for you, but will this break other users?

@thedodd thedodd added the discussion This item needs some discussion label Jul 7, 2023
@amrbashir
Copy link
Contributor Author

For tauri on mobile, due to some limitations, the webview can't access trunk server on the local network so instead we proxy the requests by connecting to trunk from the native part (Rust) of the APK and then serving the response to the webview using an internal custom protocol, so window.location is actually an internal URL which will be http://tauri.localhost but that is not an actual dev server and so connecting to it via websockets will fail.

With this change, we modify the HTML on the fly before serving it based on the incoming request URL, so if we have trunk configured to listen on 0.0.0.0 (means to listen on all local addresses), and a tauri app tries to connect to 192.168.1.13:8080, it will be served an HTML file that has hot-reload script that tries to connect to the socket on 192.168.1.13:8080/_trunk/ws

I don't think this will break for other users but in our fields you can never be so sure, however, for [email protected], we have been recommending users to use my fork (which has this change) for quite sometime now, used with leptos, yew and sycamore, and I haven't had any reports about regressions or bugs related to this but ofc take it with a grain of salt.

@amrbashir
Copy link
Contributor Author

Is there anything I can do to push this and the related PR forward? It would be great for tauri users to have these features merged in this project so they wouldn't have to use my fork.

And I apologize if this sounds rude or impatient on my part, I appreciate your hard work and I am not trying to put more work on your plate.

@ctron
Copy link
Collaborator

ctron commented Oct 5, 2023

I cherry-picked this into trunk-ng and will test it locally. If that looks good, it should be in the next release.

@ctron
Copy link
Collaborator

ctron commented Oct 11, 2023

It's released as part of trunk-ng 0.17.10

Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Nov 26, 2023
Copy link

github-actions bot commented Dec 7, 2023

This PR was closed because it has been stalled for 10 days with no activity.

@github-actions github-actions bot closed this Dec 7, 2023
@amrbashir
Copy link
Contributor Author

damn, forgot to comment to keep this PR open, I would still like to see this PR land if possible

@ranile
Copy link
Contributor

ranile commented Dec 7, 2023

damn, forgot to comment to keep this PR open, I would still like to see this PR land if possible

Referencing #625 here as well

@ctron ctron reopened this Dec 7, 2023
@ctron
Copy link
Collaborator

ctron commented Dec 7, 2023

This should come back to trunk with #623

@github-actions github-actions bot removed the Stale label Dec 8, 2023
@amrbashir
Copy link
Contributor Author

closing as this has been superseded by #623

@amrbashir amrbashir closed this Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This item needs some discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants