chore(trace viewer): support HMR #33609
Conversation
This comment has been minimized.
This comment has been minimized.
|
I can confirm that this works. |
| @@ -48,8 +48,8 @@ const xtermDataSource: XtermDataSource = { | |||
|
|
|||
| const searchParams = new URLSearchParams(window.location.search); | |||
| const guid = searchParams.get('ws'); | |||
| const wsURL = new URL(`../${guid}`, window.location.toString()); | |||
| wsURL.protocol = (window.location.protocol === 'https:' ? 'wss:' : 'ws:'); | |||
| const wsURL = new URL(`/${guid}`, searchParams.get('server') ?? window.location.toString()); | |||
There was a problem hiding this comment.
I am afraid that /guid top-level path will not work for non-top-level UI mode, for example UI mode forwarded from codespaces?
There was a problem hiding this comment.
In the common case, this will use the URL from ?server as the base URL. That URL doesn't have any path anyways, so ../${guid} will resolve to the same as /${guid}. As far as I know, codespaces doesn't perform any URL manipulation, but instead forwards based on ports, so this should be fine. Let me test that though.
There was a problem hiding this comment.
ToT is broken in codespaces. If you run npx playwright test --ui --ui-host=localhost, codespaces will open up https://fantastic-space-goggles-x5p9q4v5v4g36xxx-45079.app.github.dev/trace/uiMode.html?ws=...&server=http://localhost:45079. It's the same path as outside of codespaces, but a different hostname. It loads all the static assets fine, but the server parameter continues pointing to localhost, which it cannot access.
| const urlPath = `./trace/${options.webApp || 'index.html'}?${params.toString()}`; | ||
| let baseUrl = '.'; | ||
| if (process.env.PW_HMR) | ||
| baseUrl = 'http://localhost:44223'; // port is hardcoded in build.js |
There was a problem hiding this comment.
Don't we need ?server parameter? Perhaps I've lost where it is added, could you please add a comment explaining?
|
I've rebased this onto the new changes in |
This comment has been minimized.
This comment has been minimized.
packages/trace-viewer/src/sw/main.ts
Outdated
| if (traceViewerServerBaseUrl.searchParams.has('server')) | ||
| traceViewerServerBaseUrl = new URL(traceViewerServerBaseUrl.searchParams.get('server')!, traceViewerServerBaseUrl); | ||
| const clientURL = new URL(client?.url ?? self.registration.scope); | ||
| let traceViewerServerBaseUrl = new URL('../', clientURL); |
There was a problem hiding this comment.
IIUC, we are trying to support relative server url here? If so, perhaps it should be relative to the clientURL instead of '../'? I find this contract to be harder to understand.
Basically:
const traceViewerServerUrl = new URL(clientURL.searchParams.get('server') ?? '../', clientURL);There was a problem hiding this comment.
That works, sure! We're not currently using relative server anyways, happy to change this to anything.
There was a problem hiding this comment.
done, implemented your recommendation
| @@ -48,8 +48,8 @@ const xtermDataSource: XtermDataSource = { | |||
|
|
|||
| const searchParams = new URLSearchParams(window.location.search); | |||
| let testServerBaseUrl = new URL('../', window.location.href); | |||
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"1 failed 3 flaky37127 passed, 650 skipped Merge workflow run. |
Adds HMR support to the trace viewer. Hopefully this is the final PR in this saga.