Skip to content
This repository was archived by the owner on May 13, 2025. It is now read-only.

Conversation

@praveen5959
Copy link
Contributor

No description provided.

Copy link
Contributor

@balaji-jr balaji-jr left a comment

Choose a reason for hiding this comment

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

Let's implement react-query like everywhere else in this project.
This behavior is handled internally in react-query.

@praveen5959 praveen5959 requested a review from balaji-jr October 23, 2024 11:18
Copy link
Contributor

@balaji-jr balaji-jr left a comment

Choose a reason for hiding this comment

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

If we use react-query, why'd we still need to perform abort on signal ?

@praveen5959
Copy link
Contributor Author

If we use react-query, why'd we still need to perform abort on signal ?

react-query will internally cancel the queries. React Query will stop handling the response from the previous query, but it doesn't automatically cancel the underlying network request.

To explicitly cancel the request over network. I had to use AbortController (or Axios cancelToken) to cancel the pending requests over network to save bandwidth

cc: @balaji-jr

@praveen5959 praveen5959 requested a review from balaji-jr October 23, 2024 15:10
Copy link
Contributor

@balaji-jr balaji-jr left a comment

Choose a reason for hiding this comment

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

Leaving it to react query wont do any harm neither does it improve performance by cancelling requests. Please remove the abort controller implementation.

@nitisht
Copy link
Member

nitisht commented Oct 23, 2024

Leaving it to react query wont do any harm neither does it improve performance by cancelling requests. Please remove the abort controller implementation.

AbortController seems like a cleaner approach here right. Basically it cleanly stops any overhead related to the request. If this doesn't have any overhead, why remove it?

@balaji-jr
Copy link
Contributor

Leaving it to react query wont do any harm neither does it improve performance by cancelling requests. Please remove the abort controller implementation.

AbortController seems like a cleaner approach here right. Basically it cleanly stops any overhead related to the request. If this doesn't have any overhead, why remove it?

@nitisht
Why we shouldn't abort a network request:
https://tanstack.com/query/latest/docs/framework/react/guides/query-cancellation#default-behavior - Package docs.

In addition to the above ^,
We have an abort controller in place for GRPC requests from the console. But for these rest API calls, the backend will still process the call even if we abort.

@praveen5959 praveen5959 requested a review from balaji-jr October 24, 2024 04:36
Copy link
Contributor

@balaji-jr balaji-jr left a comment

Choose a reason for hiding this comment

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

Now there are two sources of truth for the tile data. The dashboard store and the useQuery. Lets use the data from the store in the tile since we use the same in edit form / exports etc.

@praveen5959 praveen5959 requested a review from balaji-jr October 24, 2024 11:20
@nitisht nitisht merged commit 51c1fc1 into parseablehq:main Oct 25, 2024
1 of 3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants