Refactor API server to use a redirect page instead of proxy, and fix a bug in cockpit #1513
Conversation
|
A couple of things to be aware of: Sometimes updates to embark's and blockchain's process logs, as seen in cockpit, don't start happening immediately after loading cockpit (reason unknown) but most of the time they do start right away. There are some problems with how we're escaping characters like Neither issue pertains directly to the code changed in this PR, so they'll be addressed in later PRs. |
michaelsbradleyjr
force-pushed
the
fix/ui-proxy
branch
from
d50014a to
16cd846
Compare
| @@ -179,7 +179,7 @@ export function listenToChannel(credentials, channel) { | |||
| } | |||
|
|
|||
| export function webSocketProcess(credentials, processName) { | |||
| return embarkAPI.webSocketProcess(credentials, `/process-logs/${processName}`); | |||
There was a problem hiding this comment.
Was there a double-up of process-logs in the failing URLs before? For example, embark-api/process-logs/process-logs/blockchain. I guess I'm curious as to why there was no need for an update on the other end (ie in embark-api-client). At a glance, what you've done looks correct.
There was a problem hiding this comment.
There was. I've seen them too, but they didn't seem to cause errors in the network, which is why I wasn't sure if it was really an issue or just devtools just outputting certain protocol request in a different way...
There was a problem hiding this comment.
@emizzle yes, in the network tab of browser devtools they looked like process-logs/[name]//process-logs/[name].
@PascalPrecht they didn't cause errors per se, but because of the bad URLs the process logs weren't getting updates. You could toggle between the process log tabs and a fetch would result in the latest logs being loaded; but with the URL fix the updates come streaming in, which is the desired behavior.
There was a problem hiding this comment.
So really there were two bugs. One was related to the proxy on 55555 forwarding /embark-api/ requests; the other was the bad URLs, unrelated to the first. The effect of the second wasn't apparent until the first one was fixed.
| <p>to instead use a static build from the monorepo, restart embark with: | ||
| <code>EMBARK_UI_STATIC=t embark run</code></p> | ||
| ` : ""} | ||
| <p>this page will automatically redirect to <a id="redirect" href=""> | ||
| <span></span></a> in <span id="timer">${redirectSeconds}</span> |
There was a problem hiding this comment.
The <span> inside the <a> is useless as it's overwritten
There was a problem hiding this comment.
Updated, and I also made a change so that loading from 127.0.0.1:55555 works just as well as loading from localhost:55555.
Embark API server's development proxy from port 55555 to 3000 was attempting to inappropriately forward an `/embark-api/` endpoint for the blockchain process logs to Create React App's development server. Why it was only happening for the one endpoint is not known but probably has to do with timing around registration of the API server's express routes. The problem can be fixed with a one-line `filter:` function in the options for `express-http-proxy`. However, it was realized that to fix an unrelated problem, whereby the proxy doesn't forward websockets to CRA such that hot reload doesn't work when accessing `embark-ui` in development on port 55555, a switch to `http-proxy-middleware` would be required. That was quickly attempted (easy switch) but there are outstanding [difficulties][bug] with `webpack-dev-server` and `node-http-proxy` that cause CRA to crash. Switch strategies and refactor the API module to serve a page on port 55555 (in development only) that alerts the developer `embark-ui` should be accessed on port 3000. The page redirects (client-side) after 10 seconds, with URL query params and/or hash preserved. A future version could instead do client-side polling of port 3000 with `fetch` and then redirect only once it's available. The reason for not redirecting immediately is that the intermediate page makes it more obvious what needs to be done, e.g. CRA dev server may need to be started with `yarn start`. [bug]: webpack/webpack-dev-server#1642
`webSocketProcess` function in `embark-api-client` expects only a process name, but cockpit was calling it with name-strings prepended with `/process-logs/` resulting in bad URLs. Revise cockpit's `services/api` module to call `webSocketProcess` with process names only.
michaelsbradleyjr
force-pushed
the
fix/ui-proxy
branch
from
16cd846 to
f9733d0
Compare
refactor(@embark/api): in dev use cockpit redirect instead of proxy
Embark API server's development proxy from port 55555 to 3000 was attempting to inappropriately forward an
/embark-api/endpoint for the blockchain process logs to Create React App's development server. Why it was only happening for the one endpoint is not known but probably has to do with timing around registration of the API server's express routes.The problem can be fixed with a one-line
filter:function in the options forexpress-http-proxy. However, it was realized that to fix an unrelated problem, whereby the proxy doesn't forward websockets to CRA such that hot reload doesn't work when accessingembark-uiin development on port 55555, a switch tohttp-proxy-middlewarewould be required. That was quickly attempted (easy switch) but there are outstanding difficulties withwebpack-dev-serverandnode-http-proxythat cause CRA to crash.Switch strategies and refactor the API module to serve a page on port 55555 (in development only) that alerts the developer
embark-uishould be accessed on port 3000. The page redirects (client-side) after 10 seconds, with URL query params and/or hash preserved. A future version could instead do client-side polling of port 3000 withfetchand then redirect only once it's available. The reason for not redirecting immediately is that the intermediate page makes it more obvious what needs to be done, e.g. CRA dev server may need to be started withyarn start.fix(@cockpit/services): send only process names to embark-api-client
webSocketProcessfunction inembark-api-clientexpects only a process name, but cockpit was calling it with name-strings prepended with/process-logs/resulting in bad URLs. Revise cockpit'sservices/apimodule to callwebSocketProcesswith process names only.