Refactor Discover/Server/TestConnection#34952
Conversation
There was a problem hiding this comment.
Regarding "reverse hook drilling": half of what useTestConnection does is calling useConnectionDiagnostic to pass its return values up.
Instead, TestConnection can call both useConnectionDiagnostic and useTestConnection. If useTestConnection needs some stuff that useConnectionDiagnostic returns, TestConnection can pass it as an argument to useTestConnection.
This makes it easier to see what hooks actually depend on and makes it easier to reuse those hooks in different places.
In this case, I removed useTestConnection altogether and move its contents to TestConnection directly. The only two other things it was doing were:
- Creating
startSshSessionandtestConnection: these can be shared as functions once I get around to implementingTestConnectionfor Connect My Computer. - Preparing logins: this was inlined and logins will be fetched through other means in the Connect My Computer connection test.
There was a problem hiding this comment.
The same pattern exists in web/packages/teleport/src/Discover/Kubernetes/TestConnection/useTestConnection.ts and web/packages/teleport/src/Discover/Database/TestConnection/useTestConnection.t, do you plan on refactoring those as well?
There was a problem hiding this comment.
I don't, I wanted to refactor the SSH component so that I can reuse some parts for Connect My Computer connection test more easily.
But I'll add a deprecation comment to those hooks with a link to this PR.
There was a problem hiding this comment.
This is another good example of "reverse hook drilling". useConnectionDiagnostic calls useTeleport and then passes clusterId up. With useTestConnection inlined into TestConnection, TestConnection could instead call both useTeleport and useConnectionDiagnostic instead.
We could even have useClusterId to avoid having to go through useTeleport().storeUser.getClusterId. But in this case I found out that clusterId can be accessed without calling any hooks. Other connection tests depend on clusterId from here though, so for now I just marked it as deprecated.
ravicious
added
the
no-changelog
ravicious
requested review from
ibeckermayer
and removed request for
rudream
ravicious
force-pushed
the
ravicious/refactor-test-connection
branch
from
2392827 to
5c30ead
Compare
| import { TestConnection } from './TestConnection'; | ||
|
|
||
| export default { | ||
| title: 'Teleport/Discover/Shared/ConnectionDiagnostic/Server', |
There was a problem hiding this comment.
BTW, wouldn't it be more convenient if Teleport/Discover/Shared/ConnectionDiagnostic held stories only for code that is shared, but connection tests for specific resources were under Teleport/Discover//TestConnection?
Otherwise you have to jump around to find the story for the connection test vs having everything for a specific resource under a single category.
This is how it looks now:
A screenshot of Storybook sidebar with stories for Discover
There was a problem hiding this comment.
yes that's better. i don't remember why i did it like that 🤷♀️
There was a problem hiding this comment.
The same pattern exists in web/packages/teleport/src/Discover/Kubernetes/TestConnection/useTestConnection.ts and web/packages/teleport/src/Discover/Database/TestConnection/useTestConnection.t, do you plan on refactoring those as well?
kimlisa
left a comment
kimlisa
left a comment
There was a problem hiding this comment.
thanks for the refactor 👍
| import { TestConnection } from './TestConnection'; | ||
|
|
||
| export default { | ||
| title: 'Teleport/Discover/Shared/ConnectionDiagnostic/Server', |
There was a problem hiding this comment.
yes that's better. i don't remember why i did it like that 🤷♀️
|
@ravicious See the table below for backport results.
|
3 participants
The TestConnection component for Connect My Computer will look very similar to the Server. However, before jumping in and adding a test connection for Connect My Computer, I first wanted to refactor Server/TestConnection away from the container pattern.
Along the way I made a couple of other changes, most significant being the removal of what could be called "reverse hook drilling" in
useTestConnection. For now I inlined its implementation intoTestConnection. The connection test for Connect My Computer will likely share some implementation through also callinguseConnectionDiagnostic, but functions likestartSshSessionwill be shared as just regular functions, without using hooks.