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

useBackgroundQuery + refetch Suspense UI bug? (pull to refresh ScrollView in RN + Expo) #11243

Open
mvolonnino opened this issue Sep 22, 2023 · 8 comments

Comments

@mvolonnino
Copy link

mvolonnino commented Sep 22, 2023

Been digging for hours and cant find anything on this issue.

I have a useBackgroundQuery + a full screen Suspense loading fallback. Everything works as expected on first load, the fallback skeleton shows when the App loads. I have a ScrollView wrapping children that utilize useReadyQuery with the queryRef passed for grabbing the data when the query has resolved.

Problem arises is when I pull to refresh (grab new data from the useBackgroundQuery to replenish the components that need to display anything new from the server).

On that pull to refresh, the suspense fallback UI shows, along with the screen components as well and it is some poor UI. I would expect this to work like a normal useQuery, where I can utilize the refetch in juncture with a refreshControl prop where I pull to refresh, the RefreshControl shows, while keep the loading from the useQuery as false to keep the UI of loading to the RefreshControl, and the components update when the refetching has complete.

I see this pr: #10809 which shows a fix for useSuspenseQuery but not sure if this pertains to the useBackgroundQuery

Also read this from the docs: https://www.apollographql.com/docs/react/data/suspense/#refetching-and-pagination

minimal code repro:
"@apollo/client": "^3.8.1",
"expo": "^49.0.0",
"react-native": "0.72.4",

const HomeContent = ({ scrollHandler }: HomeProps) => {
  const [refreshing, setRefreshing] = useState(false);
  const [isPending, startTransition] = useTransition();

  const {
    cityPreference: { cityName },
  } = useCityPreferenceSlice();
  const [queryRef, { refetch }] = useBackgroundQuery(GET_RECOMMENDER_AND_STAFF_PICKS, {
    errorPolicy: 'none', // to allow the error to be caught by the closest ErrorBoundary in the tree
    variables: { input: { cityName: cityName! } },
  });

  const pullToRefresh = () => {
    startTransition(() => {
      try {
        // setRefreshing(true);
        refetch();
      } catch (error) {
        console.warn(`Error while refetching: ${error}`);
      } finally {
        // setRefreshing(false);
      }
    });
  };

  return (
    <HomeCityBoundary
      fallback={
        <FadeInOutView flex={0.8} style={{ justifyContent: 'center', alignItems: 'center' }}>
          <Heading type="section">No City Found</Heading>
          <Text type="caption" fontSize={16} textAlign="center">
            Please select a city from the dropdown above to see whats happening in your area
          </Text>
        </FadeInOutView>
      }
    >
      <AnimatedTabScrollView
        withTabBarHeightAdjustment
        contentContainerStyle={{ paddingTop: 10 }}
        refreshControl={<RefreshControl refreshing={isPending} onRefresh={pullToRefresh} />}
        onScroll={scrollHandler}
        decelerationRate={0.9998}
      >
        <View px={2} my={1}>
          <DeepLinkGrid />
        </View>
        <View my={5}>
          <RecommendedList queryRef={queryRef} />
        </View>
        {/* placeholder to allow extra scroll for abs positioned VoloPassCTA */}
        <View height={5} />
      </AnimatedTabScrollView>
    </HomeCityBoundary>
  );
};

/**
 * This is the main entry point for the Home screen
 * - Handles the `error` boundary for the entire screen
 * - Handles the `suspense` fallback (loading) for the entire screen
 */
const Home = () => {
  // scrollY for the VoloPassCTA which animates to a smaller size as the user scrolls down
  const scrollY = useSharedValue(0);
  const scrollHandler = useAnimatedScrollHandler(
    {
      onScroll: nativeEvent => {
        scrollY.value = nativeEvent.contentOffset.y;
      },
    },
    []
  );
  // make request for membership details to show/hide VoloPassCTA
  // this should cache for the lifetime of the users session
  const { loading } = useMembershipDetails();

  return (
    <FadeInOutView>
      <Header />
      <ErrorBoundary FallbackComponent={GenericErrorFallback}>
        <Suspense fallback={<HomeSkeleton />}>
          <FadeInOutView>
            <HomeContent scrollHandler={scrollHandler} />
          </FadeInOutView>
        </Suspense>
      </ErrorBoundary>
      {!loading && (
        <SpringyFadeInDownView flex={0}>
          <PositionedVoloPassCTA scrollY={scrollY} />
        </SpringyFadeInDownView>
      )}
    </FadeInOutView>
  );
};

export default Home;

I originally had it setup with a refreshing state within the HomeContent, and would update that on pull to refresh. This works as expected as well without any of the useTranistion (implemented from finding this issue) hooks in Expo Go, but when building expo-dev-client and an EAS build for internal testing, the issue happens.

You can see that the HomeContent is wrapped with Home, which holds the , when i pull to refretch on the ScrollView, the HomeContent stays rendered, but the Suspense fallback is also rendered creating a very jarring / poor UI experience which DOESNT happen in Expo Go, but does when building with EAS build ( both with dev client true, and a preview build)

Anyone come across this issue or am I miss reading the docs somehow?

Tasks

No tasks being tracked yet.
@jerelmiller
Copy link
Member

Hey @mvolonnino 👋

The way you setup the code looks correct to me. startTransition should prevent the fallback UI from showing when suspended. Your structure is also similar to how our test is setup for refetch + startTransition. The only difference I see is that the startTransition is used in the child component in our test, but I don't think this should matter. By chance, would you be able to send us a reproduction? We can dig into this a little further. I don't know if this is specific to React Native or if this affects all usages of this hook.

Thanks for reporting!

@mvolonnino
Copy link
Author

mvolonnino commented Sep 23, 2023

@jerelmiller - sure thing! this would be my first time doing something like this, what do you mean a reproduction?

Also - a quick thing to note, isPending is always false, even when I pull to refresh. Also find this structure a little odd (first time using useTransition but would think that it should be an async call so we could await the outcome of the refetch promise?

Will do more digging for that as well

*update

  • Tested it the way your test is setup as well, with the refetch being passed to child, setup up a simple button within the <RecommendedList queryRef={queryRef} refetch={refetch} /> with the useTransition hook + the startTransition utilized there and still same issue, fallback UI is still shown and isPending is always false
const RecommendedList = ({ queryRef, refetch }: RecommendedListProps) => {
  const data = useRecommender(queryRef);
  const [isPending, startTransition] = useTransition();

  const renderItem: ListRenderItem<RecommendedListItem> = useCallback(
    ({ item }) => <HomeCard {...item} />,
    []
  );

  const refresh = () => {
    startTransition(() => {
      refetch();
    });
  };

  return data.length ? (
    <>
      <View px={4} mb={4}>
        <Heading type="section">Recommended for You</Heading>
        <Button onPress={refresh} opacity={isPending ? 0.3 : 1}>
          Refresh
        </Button>
      </View>
      <HorizontalList
        data={data}
        renderItem={renderItem}
        pagingEnabled
        snapToInterval={SNAP_WIDTH} // size of the item including margins
        snapToAlignment="center" // to center the card while scrolling
        decelerationRate={0.95} // this makes scrolling snap more quickly
        style={{ width: WIDTH }}
      />
    </>
  ) : null;
};

*update 2
I know this is getting a bit much for information - apologies if I should open more issues or not, but it seems if i use fetchMore instead of refetch in the original setup I sent, the fallback UI doesn't show until after it looks like the fetchMore is done and resolves, which continues to show the RefreshControl activity indicator, but once its complete, the fallback UI shows again like a flicker - then the HomeContent is re-rendered with the new data within my RecommendedList - I would think that no fallback UI should show, rather that once the 'refresh' is done, the RefreshControl activity indicator goes away and the RecommendedList is updated with the new data

update 3
To clarify, using Expo (EAS builds in a developmentClient: true, built for device
not simulator), Expo SDK 49, Expo Router v2. When building with Expo Go (which is different from EAS builds) funny enough, on the iOS sim or device, this flicker issue does not happen (don't even need the startTransition, I was using a refresh state and just set that true / false upon a async callback wrapped around refetch)

 const pullToRefresh = async () => {
    try {
      setRefreshing(true);
      await refetch();
    } catch (error) {
      console.warn(`Error while refetching: ${error}`);
    } finally {
      setRefreshing(false);
    }
  };

that works within Expo Go on both iOS sim and device (albeit the UI flickers on Android expo go (only tested on device, the simulator does not work very well at all on my laptop)).

After building with the expo-dev-client which is what I will switch to as I will be using Native Modules from Firebase (Expo Go will no longer work for me), I started to run into that UI flicker issue upon pull to refresh of my Home tab - which prompted me to look into the useTransition and follow the new docs within Apollo I linked in my first comment.

Now in both of my EAS builds (expo-dev-client) which is much closer to a true React Native release App (and what will be on the App stores), the UI flicker problem happens in every way i've tried so far!

Hope that is a little clearer on the circumstance of this issue 🤣

@mvolonnino
Copy link
Author

@jerelmiller @bignimbus - just reaching out to see what else is needed from me in terms of this issue. Would love to get these new useBackgroundQuery + useReadQuery into production App, but with this current issue its a pretty big blocker.

Let me know if theres anything I can do!

@jerelmiller
Copy link
Member

@mvolonnino apologies I've been slow to respond. I was at a conference last week and am gearing up for another one next week. Hoping I have some time to dig into this more sometime soon!

@mvolonnino
Copy link
Author

@jerelmiller - just checking in and seeing if this has had any traction? Don't want to reiterate anything said already and I obviously know how busy the maintainers of this library are, but would love to get this into production 🚀

Let me know how its going - and if I can do anything else.

@jerelmiller
Copy link
Member

@mvolonnino Apologies again! I promise this is on my todo list, but I just won't be able to promise anything for likely another few weeks. I'm speaking at my 3rd conference this week in the last 4 weeks and have 2 more in November, so I've been traveling quite a bit and have not had the time I had hoped to really dig into this stuff.

React Native has been a tricky one as we've seen quite a few funky issues with it not working quite as intended in situations where it works perfectly fine on the web.

You're using the New Architecture correct? I came across something recently that mentioned this was crucial to getting Suspense working properly (I can't seem to find the link, apologies).

If you're keen on seeing some progress sooner, feel free to dig into the useBackgroundQuery + useReadQuery hooks and see if you're able to figure out what might be going wrong. I can give you some pointers on where to begin. No pressure to do this, but this might speed up a root cause/fix if you're needing this sooner than I can promise you.

So with that, here is how I'd recommend digging into the issue:

Try your example with useSuspenseQuery first. See if startTransition is working here as expected, or whether this is isolated to useBackgroundQuery + useReadQuery. useBackgroundQuery transports some values through the QueryReference instance to useReadQuery that works slightly differently than with useSuspenseQuery.

If you're still seeing the same issues with useSuspenseQuery, here is what I'd look for next.

The way startTransition works is that it will look at the state updates caused by the callback for startTransition and see if any of those updates will cause the component to suspend. If the state updates within the startTransition callback cause the component to suspend, the existing UI is instead kept and the suspended component is moved to a background render. A key piece of this working is that your app can simultaneously be in 2 states at once. React will render your component with existing props + state (showing you the existing UI) and will render your component with new props + state in a background render. Its when that background render suspends that determines if React will hide the loading state and show you the existing UI. When React has determined that the background render is complete (i.e. it has finished suspending), it will merge that props + state update into the "main" render.

This is one of the key pieces to how Apollo provides Suspense support in these hooks. It needs to be able to handle being in the "old" and "new" states at the same time. This is done by providing a promiseCache in useState that is updated when you refetch. This gives React the ability to render your component with the old promiseCache, which should contain a resolved promise for queryRef.key, and the new promiseCache, which should contain a pending promise for that same queryRef.key (this is the big reason the Map is recreated instead of mutated in that state update).

That promiseCache is transported through QueryReference and read by useReadQuery. This promise is then passed to __use which does some unwrapping of the promise to determine whether to throw it (i.e. "suspend" the component) or return the resolved value.

Essentially we need to figure out if that promise is actually pending when the refetch call is made and the component is rerendered. The QueryReference class handles all the promise resolution logic, so if you're not seeing a pending promise when you expect to you'll need to dig into this a bit further to determine why its not pending at the right time.

Hopefully this all makes sense!


Again, don't feel like you need to dig in, but hopefully this gives you a starting point if you're interested and would like to move this along faster. I'm sorry I can't provide more help right now but conferences + priorities right now are making it difficult to get to.

@jerelmiller
Copy link
Member

jerelmiller commented Oct 17, 2023

Edit: I found this discussion which has some mentions that using useTransition to hide a Suspense fallback isn't quite working correctly. Perhaps this is a React Native bug and not an Apollo one?

@bignimbus bignimbus removed the 🏓 awaiting-team-response requires input from the apollo team label Oct 18, 2023
@jerelmiller jerelmiller added 🏓 awaiting-contributor-response requires input from a contributor and removed 🏓 awaiting-contributor-response requires input from a contributor labels Dec 14, 2023
@anidello
Copy link

anidello commented Sep 17, 2024

Hi, I am also having an issue with refetch, startTransition and React Native.
I actually copied your Apollo Client refetch test inside our codebase and adapted to React Native render etc and it does indeed show the Suspense Fallback UI !

So it DOES seem an issue with React Native.

UPDATE: it is -> facebook/react-native#42101 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants