Details
Created this dummy params override object:
const dummyOverride = {
urlPrefixOverride: 'testing',
};And passed it to endpoint calls that would normally use an established socket and confirmed that when we overrode the url prefix with this dummy params override object we were not entering the condition in callServerEndpoint where we use an established socket
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
After merging + rebasing @inka's changes where she introduced the useGetVersion hook, we now use this hook instead of using useServerCall(getVersion). When the useGetVersion hook calls the callServerEndpoint function it will skip using the established socket by default, making this diff not necessary anymore
| lib/utils/call-server-endpoint.js | ||
|---|---|---|
| 109 ↗ | (On Diff #32612) | Should this change still be made? |
| lib/utils/call-server-endpoint.js | ||
|---|---|---|
| 109 ↗ | (On Diff #32612) | No this is no longer necessary since we don't override the urlPrefix directly now. Instead we override the map of keyserverInfos (which contains the new url prefix) with entries for each keyserver we want to connect to. From there we use that map of overridden keyservers to build our map of requests to each respective keyserver. How we override the map of keyserverInfos: Building our map of requests: Not sure if this is super relevant to the original question but here's a helpful comment by @inka explaining how we should be using the useGetVersion hook |
| lib/utils/call-server-endpoint.js | ||
|---|---|---|
| 109 ↗ | (On Diff #32612) | I get that we're not using it, but shouldn't this be the behavior when urlPrefixOverride is set? If we use the socket when urlPrefixOverride is set, won't that prevent urlPrefixOverride from working? Unless I'm missing something, I think we either need to still make this change, or we need to deprecate urlPrefixOverride to prevent somebody from using it in the future, as it appears to me that it's very broken (will be straight up ignored if a socket is connected) |
| lib/utils/call-server-endpoint.js | ||
|---|---|---|
| 109 ↗ | (On Diff #32612) | I've thought about this some more, and I was confusing myself by overthinking this just in the context of this get version call. My bad. The intention of this diff should be that whenever we are overriding urlPrefix we never want to use an established socket since that socket won't respect the overridden urlPrefix. So yes we should still check for this so we don't use an established socket if we ever make a server call in the future with an overridden url prefix. |