optimise(sdk): change the socket host url support the doctor used in devcontainer (Docker)

easy1090 commented 9 months ago

Summary

Checklist

[x] I have added changeset via pnpm run change.
[ ] I have updated the documentation.
[ ] I have added tests to cover my changes.

netlify[bot] commented 9 months ago

Deploy Preview for rsdoctor ready!

Name	Link
Latest commit	6677cc33f5623d8d68c145bad30734db49a98ab6
Latest deploy log	https://app.netlify.com/sites/rsdoctor/deploys/65c4947617c9bd000825103c
Deploy Preview	https://deploy-preview-181--rsdoctor.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

my-lalex commented 9 months ago

Reading quickly, this should work... but seams a little messy... The good thing I didn't see when posting #161 is that the client was already using location.host, which is basically the same as using relative path but necessary 'cause you have to change protocol so a full URL is needed...

What seem a bit messy to me, is using back ip.address() then url.replace('localhost', host) on your openClientPage method... Wouldn't it be cleaner to have a method returning an object with host, port, path, qs, then assemble it when you need the URL (allowing to create easily get URL for http and ws), and use it twice with localhost instead of host the second time when you have to display both URLs?

It's just my 2 cts: having this issue fixed this way is already great, thanks! 😁

easy1090 commented 9 months ago

your openClientPage method... Wouldn't it be cleaner to have a method returning an object with host, port, path, qs, then assemble it when you need the

You are right, I will change this code. Thanks~

web-infra-dev / rsdoctor