Refactor REST client #286
Refactor REST client #286
Conversation
code-asher
force-pushed
the
asher/refactor-client
branch
from
11ab645 to
a07603b
Compare
code-asher
force-pushed
the
asher/refactor-client
branch
3 times, most recently
from
b926f76 to
c5ec0c7
Compare
We need this to create standalone clients so we can support multiple deployments. Currently the plugin always uses the url and token for the current deployment, but you might be trying to launch a previous workspace that belongs to a different deployment (from the recents projects menu for example). Also, lots of the code relies on the url and token being stored and accessed globally (storage.getURL(), storage.getSessionToken()) so avoid those as much as possible (only use when creating a client, basically).
These should work now once the clients are pointing to different things.
code-asher
force-pushed
the
asher/refactor-client
branch
from
c5ec0c7 to
f6bafef
Compare
| restClient.setSessionToken(token) | ||
| } | ||
|
|
||
| restClient.getAxiosInstance().interceptors.request.use(async (config) => { |
There was a problem hiding this comment.
Not a huge deal, but is there a reason why the Axios instance isn't a separate variable?
There was a problem hiding this comment.
Though I guess another option would be to add both interceptors in one call of interceptors.request.use
There was a problem hiding this comment.
Oh like why not split out into const axios = restClient.getAxiosInstance() type of thing?
Could do that, I tend to like leaving things on their objects and accessing them that way, mostly just a preference thing I think. I think maybe I like having them "namespaced" or something like that.
Edit: to add that this only applies to simple getters, if the function does work or the return value can change I would encapsulate into a var for sure
There was a problem hiding this comment.
Oh and one is a request interceptor and the other a response interceptor so I think they have to be separate calls
There was a problem hiding this comment.
Oh, wait – you're right. Totally missed that they were different names
| @@ -277,6 +303,7 @@ export class Remote { | |||
| agent = matchingAgents[0] | |||
| } | |||
|
|
|||
| // Do some janky setting manipulation. | |||
code-asher
force-pushed
the
asher/refactor-client
branch
from
83fa379 to
96c2642
Compare
We support connecting to previous workspaces, but sometimes this breaks because the previous workspace belongs to a different deployment, and the plugin always uses the credentials of the current deployment (#173).
This is a first step to fixing that: separating the client into two: one used by the plugin as a whole (mainly used to list workspaces of the current deployment in the sidebar) and one used to connect to a workspace which may belong to a different deployment (which is used to start the workspace, get the version, etc). Thankfully this is pretty chill since @Parkreiner's changes to Coder core!
For now that second client does still use the global url and token so functionally it is still the same, but I will fix that in a follow up PR by looking at the host name and figuring out the right credentials to use for it.
As part of this, I try to remove usage of a few globals, in particular we had stuff like
storage.getSessionToken()all over the place, now I only call that once when creating a client and we use it for the lifetime of the task.Alternatives
Instead of creating a separate client, we could log the entire plugin in to the right deployment. So if you are connected to deployment A, then connect to a workspace belonging to deployment B, the plugin would now entirely be logged into deployment B.
That felt a little too unexpected to me, so I went with the multiple client route.
But maybe it would be better to do a full login, because then the workspaces you see in the sidebar belong to the same deployment.
Still, I think these changes are generally good anyway because they are getting rid of a bunch of global stuff. We can do a full login if we want to with these changes as well.