-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
🪟 🔧 Remove useRouter hook [to be closed] #14869
Conversation
sortBy?: string; | ||
} | ||
|
||
export const useRouterQuery = (): UseRouterQueryResult => { |
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.
I feel we should not need this anymore, since we (should) know whether something comes in via query parameter or a route parameter, so that we should in the places we use this, just use the useParams
hook directly and have one custom useQuery()
hook (that doesn't mix the params into the query parameters).
I'll leave in the used places a comment, where I believ the information is coming from.
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.
read your last comment and got what you meant here
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.
so now fixed this. will left comment under every fixed router usage
|
||
export default useRouter; | ||
export const useRouterReplace = (): UseRouterReplaceInterface => { |
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.
Wonder if we really want to keep this as a custom hook around, since we anyway just use it in one place?
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.
fixed
@@ -76,7 +76,7 @@ function useWorkspaceApiService() { | |||
} | |||
|
|||
export const useCurrentWorkspaceId = () => { | |||
const { params } = useRouter<unknown, { workspaceId: string }>(); | |||
const params = useParams() as { workspaceId: string }; |
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.
useParams
support a generic paramter, and we should specify this as it's generic parameter not casting it (more type safe approach). This comment applied also for all other useParams
calls in this PR.
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.
I changed current version to something like this
export const useCurrentWorkspaceId = () => {
const params = useParams<{ workspaceId: string }>();
return params.workspaceId as string;
};
but still need to cast workspaceId
to string
since it is giving TS errors that value might be undefined. Is that okey,?
@@ -39,7 +40,8 @@ type FullTableProps = CreditConsumptionByConnector & { | |||
}; | |||
|
|||
const UsagePerConnectionTable: React.FC<UsagePerConnectionTableProps> = ({ creditConsumption }) => { | |||
const { query, push } = useRouter(); | |||
const query = useRouterQuery(); |
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.
That should purely work from query, no params mixed in.
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.
can you be more specific on this? not sure I am following the logic here
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.
fixed
@@ -31,7 +32,8 @@ interface IProps { | |||
} | |||
|
|||
const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow, onChangeStatus, onSync }) => { | |||
const { query, push } = useRouter(); | |||
const navigate = useNavigate(); | |||
const query = useRouterQuery(); |
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.
Should only be from query, not require params.
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.
can you be more specific on this? not sure I am following the logic here
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.
fixed
@@ -47,7 +50,7 @@ export const VerifyEmailAction: React.FC = () => { | |||
}; | |||
|
|||
export const FirebaseActionRoute: React.FC = () => { | |||
const { query: { mode } = {} } = useRouter<{ mode: string }>(); | |||
const { mode } = useRouterQuery(); |
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.
Should be fine only with query no params.
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.
can you be more specific on this? not sure I am following the logic here
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.
fixed
@@ -21,7 +22,8 @@ const ResetPasswordPageValidationSchema = yup.object().shape({ | |||
const ResetPasswordConfirmPage: React.FC = () => { | |||
const { confirmPasswordReset } = useAuthService(); | |||
const { registerNotification } = useNotificationService(); | |||
const { push, query } = useRouterHook<{ oobCode: string }>(); | |||
const navigate = useNavigate(); | |||
const query = useRouterQuery(); |
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.
Should be fine with query
only no params required.
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.
can you be more specific on this? not sure I am following the logic here
@@ -23,7 +23,8 @@ const LoginPageValidationSchema = yup.object().shape({ | |||
const LoginPage: React.FC = () => { | |||
const { formatMessage } = useIntl(); | |||
const { login } = useAuthService(); | |||
const { query, replace } = useRouter(); | |||
const query = useRouterQuery(); |
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.
Should be fine with just query no params.
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.
can you be more specific on this? not sure I am following the logic here
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.
Those comments all refer to #14869 (comment). I think we should remove the useRouterQuery
hook completely, and instead use either the native useParams
(if we need it from params) or a useQuery
hook (that we'd need to build, since weirdly react-router doesn't include one). Those comments all state if the places earlier using useRouterQuery
actually will require to pull in the information from the query i.el. useQuery
or from the params i.e. useParams
.
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.
yes. got you. already fixed
@@ -26,22 +26,24 @@ import SourceConnectionTable from "./components/SourceConnectionTable"; | |||
import SourceSettings from "./components/SourceSettings"; | |||
|
|||
const SourceItemPage: React.FC = () => { | |||
const { query, params, push } = useRouter<{ id: string }, { id: string; "*": string }>(); | |||
const query = useRouterQuery(); |
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.
This seems to be the only place we actually are interested in the id
from the params and not the query.
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.
can you be more specific on this? not sure I am following the logic here
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.
fixed
sortBy?: string; | ||
} | ||
|
||
export const useRouterQuery = (): UseRouterQueryResult => { |
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.
I think we should not keep this weird merge of params and queries around anymore. I think we should instead just have a very simpley useQuery
hook for getting query parameters (without merging the path params in, since weirdly react-router doesn't have a hook for that) and useParams in all places we need the params. I've checked all current usages of this hook in this PR and left a comment which one they actually need (query or params), so I couldn't find any use-case for this mix of both.
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.
all fixed
# Conflicts: # airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ConnectionPageTitle.tsx # airbyte-webapp/src/pages/DestinationPage/pages/CreateDestinationPage/components/DestinationForm.tsx # airbyte-webapp/src/pages/OnboardingPage/OnboardingPage.tsx # airbyte-webapp/src/pages/SourcesPage/pages/CreateSourcePage/components/SourceForm.tsx
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.
# Conflicts: # airbyte-webapp/src/packages/cloud/views/credits/CreditsPage/CreditsPage.tsx # airbyte-webapp/src/pages/ConnectionPage/pages/CreationFormPage/CreationFormPage.tsx
@@ -1,39 +1,19 @@ | |||
import queryString from "query-string"; |
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.
Could we maybe rename this file to useQuery
now, since it does not contain any hook named useRouter
anymore.
interface UseQueryResult { | ||
from?: string; | ||
id?: string; | ||
mode?: string; | ||
oobCode?: string; | ||
order?: SortOrderEnum; | ||
sortBy?: string; |
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.
Putting this into this central place does not feel like the best TypeScript solution. I'd suggest we instead make useQuery
to take a generic parameter, which actually then will be the return type, so that every caller of useQuery
will specify the parameters it knows exist on this page.
If we want to make it more "fail safe" here, and say we want to make sure that each of the parameters could still be undefined, we could still put this into the return type, i.e.
useQuery = <T>(): Partial<T> => {
Show 0 bytes for cancelled sync job
…alue (#16096) Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
* chore: cleanup dormant workflows * fix: restore performance test * fix: add back GKE acceptance test * ci: add cron to GKE Kube Acceptance Test
* added release notes file * changed file name * Added release notes * Edited formatting and links * Deleted extra lines and backticks * Deleted extra spaces Co-authored-by: Amruta Ranade <11484018+Amruta-Ranade@users.noreply.github.com>
Release name should have a dash instead of a period.
* enforce ssl in strict mysql and postgres source * Add test to verify tunnel aware strict SSL behavior * bump version numbers and changelogs * auto-bump connector version [ci skip] * auto-bump connector version [ci skip] Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
* Changed August to July in title, file, and sidebar * deleted extra spaces
|
I feel like this has a bad merge from master going on. |
yeah. I will create a new PR, since I am not sure how to restore this one |
created new pull request #16598 |
What
Closes #14557
How
As mentioned in the issue description, I removed
useRouter
hook and replaced it in corresponding places with direct hooks for navigation, location, params, etc. I also added two custom hooks (useRouterQuery
,useRouterReplace
), that were initially inuseRouter
, since they responsible for some logic, using direct hooks