-
Notifications
You must be signed in to change notification settings - Fork 32
chore: Convert to TS - services selfHosted, access, pull, nav #3439
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
00ad3a6
40c37a6
bbefa48
a00d1e3
9705d83
40727a2
b79034f
fbb038a
068e3f3
87d588a
5fb27b5
85ebe1d
76cff1f
1e244d4
3e4c483
88b1271
1fe356f
541f9f9
4c5c4ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ import omitBy from 'lodash/omitBy' | |
import qs from 'qs' | ||
import { useHistory, useLocation } from 'react-router-dom' | ||
|
||
export function useLocationParams(defaultParams = {}) { | ||
export function useLocationParams(defaultParams: Record<string, unknown> = {}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we can use some TS generics here so that the default params that we pass in are typed to the return value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And i wonder if we can utilize these same generics to the update functions etc. so that when we call them we know the keys, and their value types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I had it most of the way there in this file but the changes it touched started to explode to 10+ other files and fixes needed. Spun it into this ticket and I can take another stab later - codecov/engineering-team#2843 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay sounds good There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's gonna be a significant investment, we may want to skip it, and wait until we move routers to save time ... and potentially lots of hair pulling. |
||
const { push } = useHistory() | ||
const { pathname, search, state } = useLocation() | ||
const params = state || { | ||
|
@@ -12,7 +12,7 @@ export function useLocationParams(defaultParams = {}) { | |
}), | ||
} | ||
|
||
function updateWindowLocation(params) { | ||
function updateWindowLocation(params: Record<string, unknown>) { | ||
const locationParams = omitBy( | ||
params, | ||
(value, key) => value === defaultParams[key] | ||
|
@@ -22,12 +22,12 @@ export function useLocationParams(defaultParams = {}) { | |
} | ||
|
||
// Create new state | ||
function setParams(newParams) { | ||
function setParams(newParams: Record<string, unknown>) { | ||
updateWindowLocation(newParams) | ||
} | ||
|
||
// Retain previous state | ||
function updateParams(newParams) { | ||
function updateParams(newParams: Record<string, unknown>) { | ||
updateWindowLocation({ | ||
...params, | ||
...newParams, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test was wrong before