- Notifications
You must be signed in to change notification settings - Fork1.1k
Closed as not planned
Assignees
Description
Split off from#11421
Following@sreya 's lead by splitting up the custom hook issue into more manageable chunks.
I feel like I've done the most custom hook testing out of anybody in the company, so I'm happy to take this chunk, which is likely going to be the most complicated set
Global reusable hooks
| Hook name | Needs | Comments |
|---|---|---|
| useDebouncedFunction | Touch up | One of the first custom hooks tests I ever wrote |
| useDebouncedValue | Touch up | One of the first custom hooks tests I ever wrote |
| useCustomEvent | Touch up | In a great spot; some assertions could be simplified |
| useEffectEvent | Touch up | Not much to it, but it's become ingrained in many parts of the codebase already |
| useClassName | Tests | My main worry with this hook is that it's adding CSS directly to the HTML from within the render logic (it's using Vanilla Emotion, and not the React-specific flavor that usesuseInsertionEffect). Not sure if that also means that styles can never be cleaned up |
| useClickable | Tests | Could probably have some TypeScript type definitions cleaned up too (sorry) |
| useClickableTableRow | Redesign or deletion | This is causing more accessibility problems than it's solving. See#12248 |
| useClipboard | Tests | Especially important, since we had a regression around this |
| usePaginatedQuery | Re-review | Might be good to go; there are a lot of tests for this already. By far the most complicated hook in this group |
| usePagination | Tests | Feels like it's right at the boundary between a custom hook and a utility function; might be better to remove the hook behavior |
| useSearchParamKey | Tests | PreviouslyuseTab; got beefed up and generalized |
| useTime | Tests | Hook just got added. Wondering if we could start using it to get rid of (or at least minimize) some of the time-based side effects we're baking directly into render logic... |
| useWorkspaceBuildLogs | Tests | Uses websockets under the hood – not sure how tough that will be to deal with, considering that MSW apparently hasn't added support yet |
Other hooks
I'm also including this other hook with this group, because it's used in a few different spots, and I think updating it could require a bit of in-depth knowledge of React rendering behavior:
| Hook name | Filepath | Needs | Comments |
|---|---|---|---|
| useProxyLatency | ./src/contexts/useProxyLatency | Bug fixes, possible test case touch-ups | There's some test coverage here already, but from my quick review, there are some logic bugs in the hook that need to fixed |
And throwing this one simply because it's already done, and I don't know where else to put it:
| Hook name | Filepath | Needs | Comments |
|---|---|---|---|
| useWorkspaceDuplication | ./src/pages/CreateWorkspacePage/ | Has tests | Might be worth moving this to the hooks folder? |
Task list
useDebouncedFunctionuseDebouncedValueuseCustomEventuseClassNameuseClickableuseClickableTableRowuseClipboardusePaginationusePaginatedQueryuseSearchParamKeyuseTimeuseWorkspaceBuildLogsuseProxyLatencyuseWorkspaceDuplication
Notes
- I'm going to be a bit busy with Backstage launch until next Friday, so I probably won't be able to start any other hooks until then. People are free to take any of these hooks, but I would appreciate being tagged on the PRs
- Ben and I were talking the other day about doing taking a week-long breather at some point after launch to do some tests and bug fixes in core (though if we get a bunch of Backstage feedback, that might have to wait)