Skip to content
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 #16598

Merged
merged 8 commits into from
Sep 15, 2022
Merged

Conversation

YatsukBogdan1
Copy link
Contributor

@YatsukBogdan1 YatsukBogdan1 commented Sep 12, 2022

What

Closes #14557
Replaces #14869

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 in useRouter, since they responsible for some logic, using direct hooks

@YatsukBogdan1 YatsukBogdan1 requested a review from a team as a code owner September 12, 2022 16:19
@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Sep 12, 2022
Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment on the useQuery hook that will make its usage cleaner.
Also: useTrackPageAnalytics is also an empty file that may not be needed

@@ -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 = useQuery() as { sortBy?: string; order?: SortOrderEnum };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this needs to be cast?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to generic type

import { useMemo } from "react";
import { useLocation } from "react-router-dom";

export const useQuery = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to avoid casting, you could use generics here:

export const useQuery = <T>() => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines 37 to 38
const location = useLocation();
const { pathname } = location;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

const { pathname } = location = useLocation();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@dizel852 dizel852 changed the title Remove useRouter hook 🪟 🔧Remove useRouter hook Sep 13, 2022
Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested most navigation in OSS and cloud

@YatsukBogdan1 YatsukBogdan1 merged commit dcce166 into master Sep 15, 2022
@YatsukBogdan1 YatsukBogdan1 deleted the byatsuk/remove-use-router-new branch September 15, 2022 11:20
letiescanciano added a commit that referenced this pull request Sep 15, 2022
* master: (200 commits)
  🪟 🧹 Display returned error messages on replication view (#16280)
  🎉 Source mixpanel: Use "Retry-After" header for backoff (#16770)
  🐛 Source google ads: mark custom query fields required (#15858)
  🪟 🔧Remove useRouter hook (#16598)
  CDK: improve TypeTransformer to convert simple types to array of simple types (#16636)
  CDK: TypeTransformer - warning message more informative (#16695)
  Source MySQL: Add Python SAT to detect backwards breaking changes (#16445)
  remove eager (#16756)
  bump com.networknt:json-schema-validator to latest version (#16619)
  Remove Cloud from Kafka docs (#16753)
  Normalization Summaries table and read/write methods (#16655)
  comment out flaky test suite while it is being investigated (#16752)
  Update ConfigRepository to read protocol version (#16670)
  Use LOG4J2 to wrap connectors logs to JSON format (#15668)
  Update connector catalog (#16749)
  🪟 🎨 Remove feedback modal from UI (#16548)
  Add missing env var for Kube overlays (#16747)
  Prepare for React v18 upgrade (#16694)
  🪟 🐛 Fix direct job linking to work with pagination (#16517)
  Fix formatting (#16743)
  ...
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
* Removes useRouter hook

* Removes params from useRouterQuery; Removes useRouterReplace hook

* Fix comments

* Fixes from comments

* Fixes linter error
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
* Removes useRouter hook

* Removes params from useRouterQuery; Removes useRouterReplace hook

* Fix comments

* Fixes from comments

* Fixes linter error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove useRouter hook
2 participants