refactor: try using generated react-query hooks #97

markandrus · 2023-07-14T08:32:08Z

Description

@felipecadavid I think we need to take a simpler approach to using react-query. The GraphQL code generator is already producing react-query hooks, so we should not be writing more code to wrap react-query or to use features like refetchInterval. And actually, we don't even need to depend on graphql-request, we can use fetch.

Let's please try to get a version of this working, with your React 16, 17 and 18 app changes. ~~It's not yet working, and I'm not sure why.~~ There's a lot of complicated hook logic in the components that should be simplified.

UPDATE: I got it working. Additionally, I noticed

We were not using the GraphQL code generator's msw mocks. We should always try to use these, so I added them.
Our logic for showing the loading state seems messy. We have to pairs of "is loading" values we need to check, depending on isStatic. So it becomes (isStatic && loading) || (!isStatic && isLoading). It's not super clean. We should come back and simplify this.
We lack error tests for TimeSeries and Leaderboard.

Checklist

Before merging to main:

Tests
Manually tested in React apps
Release notes
Approved

Release notes

# Component changes

## Time Series

- Added the `refetchInterval` prop, which allows configuring the interval at which the Time Series component re-fetches data from the server. By default, re-fetching is disabled.
- Added the `retry` prop, which controls whether or not errors will be retried a default number of times. By default, errors are retried.
- Removed dependency on graphql-request.
- Export TimeSeriesProps.

## Leaderboard

- Added the `refetchInterval` prop, which allows configuring the interval at which the Time Series component re-fetches data from the server. By default, re-fetching is disabled.
- Added the `retry` prop, which controls whether or not errors will be retried a default number of times. By default, errors are retried.
- Removed dependency on graphql-request.
- Export LeaderboardProps and Styles.

## Counter

- Added the `refetchInterval` prop, which allows configuring the interval at which the Time Series component re-fetches data from the server. By default, re-fetching is disabled.
- Added the `retry` prop, which controls whether or not errors will be retried a default number of times. By default, errors are retried.
- Removed dependency on graphql-request.
- Export CounterProps and Styles.

# Packages changes

## GraphQL

- Removed dependency on graphql-request.

# App changes

## React sample apps.

- Added a test in every sample app for testing `refetchInterval`.

## Storybook

N/A

linear · 2023-07-14T08:32:10Z

PRO-2152 Configurable prop for UI Kit components to refetch data

We have a customer, Christian, who says

In ui-kit version 0.26.8 the components would re-fetch pretty regularly which was useful to keep the data fresh. In version 0.31.0-rc.1, it appears that re-fetching is not occurring.
Is there a built in way to have the components re-fetch without reloading the page? Ideally setting something like a re-fetch interval.

Can we have a prop for defining a refresh interval? Ideally, we will take into consideration what other libraries do. For example, Apollo Client has a useQuery hook with startPolling and stopPolling:

image.png

react-query has refetchInterval, along with some more options for refetchOnMount, refetchInBackground, etc.:

image.png

react-query's refetchInterval is perhaps the simplest.

vercel · 2023-07-14T08:32:13Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
ui-kit	⬜️ Ignored (Inspect)	Visit Preview		Jul 17, 2023 0:54am

markandrus · 2023-07-14T10:43:42Z

packages/react/counter/src/Counter.tsx

-  const [dataValue, setDataValue] = React.useState<string | null>()
  const [propsMismatch, setPropsMismatch] = React.useState(false)
-  const [hasError, setHasError] = React.useState(false)
-  const [isLoading, setIsLoading] = React.useState(false)

-  const filtersString = JSON.stringify(query?.filters || [])
  const counterRef = React.useRef<HTMLSpanElement>(null)

-  /**
-   * Fetches the counter data
-   * when the user doesn't provide
-   * its own `value`
-   */
-  const fetchData = React.useCallback(async () => {
-    try {
-      setIsLoading(true)
-      setHasError(false)
-
-      const filters = JSON.parse(filtersString)
-
-      const response = await request<CounterQuery, CounterQueryVariables>(
-        PROPEL_GRAPHQL_API_ENDPOINT,
-        CounterDocument,
-        {
-          counterInput: {
-            metricName: query?.metric,
-            timeRange: {
-              relative: query?.timeRange?.relative ?? null,
-              n: query?.timeRange?.n ?? null,
-              start: query?.timeRange?.start ?? null,
-              stop: query?.timeRange?.stop ?? null
-            },
-            filters,
-            propeller: query?.propeller
-          }
-        },
-        {


As best I can tell, hooks for dataValue, hasError, isLoading are redundant. The useCounterQuery, generated by the GraphQL code generator, is already handling this stuff. Additionally, I do not believe we need to stringify the filters, because @tanstack/query handles that for us.

The main reason for stringifying the filters was preventing triggering the useEffect in every re-render (due to the filters array reference changing on every re-render) but yes, @tanstack/query handles that for us 😃

markandrus · 2023-07-14T10:44:44Z

packages/react/counter/src/Counter.tsx

-    query?.metric,
-    query?.accessToken,
-    query?.timeRange?.n,
-    query?.timeRange?.relative,
-    query?.timeRange?.start,
-    query?.timeRange?.stop,
-    query?.propeller,
-    filtersString


I believe @tanstack/query is tracking these dependencies for us.

markandrus · 2023-07-14T10:45:08Z

packages/react/counter/src/Counter.tsx

-  React.useEffect(() => {
-    async function setup() {
-      if (isStatic) {
-        setDataValue(value)
-      }
-
-      if (!isStatic) {
-        const fetchedValue = await fetchData()
-
-        if (fetchedValue === undefined) {
-          setHasError(true)
-          // console.error(`QueryError: Your metric ${query?.metric} returned undefined.`) we will set logs as a feature later
-          return
-        }
-
-        setDataValue(fetchedValue)
-      }
-    }
-
-    setup()
-  }, [fetchData, isStatic, query?.metric, value])


I'm pretty sure this is not necessary. Again, this is functionality @tanstack/react-query provides us.

markandrus · 2023-07-14T10:45:51Z

packages/react/counter/src/__test__/mswHandlers.ts


 const handlers = [
-  graphql.query('Counter', (req, res, ctx) => {
+  mockCounterQuery((req, res, ctx) => {


I believe we should be using the GraphQL code generator provided mock handlers. They give the best type safety.

markandrus · 2023-07-14T10:46:57Z

packages/react/time-series/src/__test__/TimeSeries.test.tsx

@@ -53,4 +53,6 @@ describe('TimeSeries', () => {
    expect(chartLabels).toEqual(timeSeries.labels)
    expect(chartData).toEqual(timeSeries.values)
  })
+
+  // TODO: Add error tests.


We should also have error tests for the Leaderboard.

felipecadavid

LGTM, I'm just going to add the test button I added in the other PR, add the error tests we are missing and fix a small issue with the static mode loading state.

…issue

felipecadavid · 2023-07-14T17:06:55Z

packages/react/counter/src/Counter.tsx

-        },
-        {
+  const {
+    isInitialLoading: isLoadingQuery,


Here I changed to isInitialLoading because isLoading will always be true when the query is disabled, this is an intended behavior from react-query that means there is no data yet. To address this kind of cases they added isInitialLoading which is a derived flag from isLoading && isFetching so it should work for us.

https://tanstack.com/query/v4/docs/react/guides/disabling-queries#isinitialloading

felipecadavid · 2023-07-14T17:36:06Z

packages/react/leaderboard/src/Leaderboard.tsx

-  React.useEffect(() => {
-    try {
-      if (variant !== 'bar' && variant !== 'table') {
-        // console.error('InvalidPropsError: `variant` prop must be either `bar` or `table`') we will set logs as a feature later
-        throw new Error('InvalidPropsError')
-      }
-      setPropsMismatch(false)
-    } catch {
-      setPropsMismatch(true)
-    }
-  }, [variant])
-


This was causing a bug, when there was a props mismatch error such as the user not passing rows but passing header for example, if the variant was correct this was setting the props mismatch error to false anyways, overriding the true value from handlePropsMismatch. To fix it, I moved this to the handlePropsMismatch function.

markandrus · 2023-07-17T12:46:33Z

I manually tested. It LGTM. But we need to make manual testing easier 🤔

refactor: try using generated react-query hooks

22d2d6e

markandrus requested a review from felipecadavid July 14, 2023 08:32

vercel bot deployed to Preview July 14, 2023 08:32 View deployment

markandrus added 2 commits July 14, 2023 12:31

test: test react-query hooks with generated msw mocks

f42131b

chore: workflow uses Node 18

24322ef

markandrus mentioned this pull request Jul 14, 2023

Configurable prop for UI kit components to refetch data #93

Closed

4 tasks

markandrus commented Jul 14, 2023

View reviewed changes

markandrus mentioned this pull request Jul 14, 2023

WIP: chore(packages): merge into ui-kit package #96

Closed

4 tasks

felipecadavid approved these changes Jul 14, 2023

View reviewed changes

refactor: add refetchInterval test in example apps and fix isLoading …

b05bea6

…issue

felipecadavid reviewed Jul 14, 2023

View reviewed changes

test: add missing error tests + refactor leaderboard bug

dd32f53

felipecadavid reviewed Jul 14, 2023

View reviewed changes

fix: correct turbo.json for test command

f54769e

chore: export types

5141f5f

markandrus merged commit 5969592 into main Jul 17, 2023
3 checks passed

markandrus deleted the pro-2152 branch July 17, 2023 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: try using generated react-query hooks #97

refactor: try using generated react-query hooks #97

markandrus commented Jul 14, 2023 •

edited

Loading

linear bot commented Jul 14, 2023

vercel bot commented Jul 14, 2023 •

edited

Loading

markandrus Jul 14, 2023

felipecadavid Jul 14, 2023

markandrus Jul 14, 2023

markandrus Jul 14, 2023

markandrus Jul 14, 2023

markandrus Jul 14, 2023

felipecadavid left a comment

felipecadavid Jul 14, 2023

felipecadavid Jul 14, 2023

markandrus commented Jul 17, 2023

refactor: try using generated react-query hooks #97

refactor: try using generated react-query hooks #97

Conversation

markandrus commented Jul 14, 2023 • edited Loading

Description

Checklist

Release notes

linear bot commented Jul 14, 2023

vercel bot commented Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipecadavid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markandrus commented Jul 17, 2023

markandrus commented Jul 14, 2023 •

edited

Loading

vercel bot commented Jul 14, 2023 •

edited

Loading