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

Use useQuery in place of usePromise for EDA subsetting and visualizations #1228

Merged
merged 30 commits into from
Nov 19, 2024

Conversation

bobular
Copy link
Member

@bobular bobular commented Oct 11, 2024

  • refactor data requests
  • make sure user db PATCH requests don't keep going forever
  • spinner behaviour
  • cancel main variable behaviour
  • check date histograms work (I saw a date-related runtime exception once when other things weren't working very well either)
  • test bad variable behaviour in MapVEu
  • barplot
  • boxplot
  • test mbio
  • lineplot
  • mosaics
  • scatter plot (maybe set cacheTime in useQuery to 60 seconds (in ms) for scatterplot?) - default is 5 minutes
  • test mbio scatter behaviour
  • other plots (volcano, networks etc) --> Upgrade MBio volcano and network visualizations to use react-query client-side caching. #1265

@bobular bobular changed the title Attempt to use useQuery in place of usePromise for EDA subsetting and visualizations Use useQuery in place of usePromise for EDA subsetting and visualizations Oct 11, 2024
@bobular bobular linked an issue Oct 25, 2024 that may be closed by this pull request
@bobular bobular requested a review from dmfalke November 18, 2024 10:54
Copy link
Member

Choose a reason for hiding this comment

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

All "MultiFilter" variables appear to be broken. An example can be found with a clinepi-site (using qa.clinepi): http://localhost:8080/a/app/workspace/analyses/DS_841a9f5259/new/variables/PCO_0000024/ENVO_00003064.

image

Copy link
Member

Choose a reason for hiding this comment

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

I think it's because one of the deps is nullish:

const enabled = queryKey.every((val) => val != null);

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @dmfalke - I have fixed this now.
I did test it but I must have already checked some checkboxes before deploying the useQuery.

Copy link
Member

@dmfalke dmfalke left a comment

Choose a reason for hiding this comment

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

I started testing with a clinepi site, and ran into an issue with a MultiFilter. See details below.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's because one of the deps is nullish:

const enabled = queryKey.every((val) => val != null);

@bobular bobular requested a review from dmfalke November 19, 2024 12:06
@bobular
Copy link
Member Author

bobular commented Nov 19, 2024

Hi @dmfalke thank you for the review.
I cleaned up a lot of dependency warnings and the multifilter bug you spotted.

Copy link
Member

@dmfalke dmfalke left a comment

Choose a reason for hiding this comment

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

I think this all looks good.

At some point, we might want to consider moving QueryClientProvider to a more central place, such as the Root component in wdk-client. However, this raises some questions about how to properly deal with dependencies like this, across packages.

@bobular bobular merged commit 42431de into main Nov 19, 2024
1 check passed
@bobular bobular deleted the 1103-react-query-retrofit branch November 19, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map and EDA: retrofit react-query's useQuery in place of usePromise
2 participants