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

Fixed the dashboard tile loading issue (#344) #346

Merged
merged 8 commits into from
Oct 25, 2024

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.

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

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.

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.

src/hooks/useDashboards.tsx Outdated Show resolved Hide resolved
@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