Fast-refresh issue

[jfrolich]

https://github.com/apollographql/apollo-client/blob/master/src/react/data/QueryData.ts#L459

^^ above line is undefined when fast refresh is refreshed. This results in errors in React-Native (or web potentially, but I experience this in React-Native). You see that there is a typescript ignore override, so please also return a valid response when this value is undefined.

[Ericnr]

I get the same error in CRA using the customize-cra-react-refresh package

[cryptedx]

Me too with RN and iOS/Android simulator and fast refresh enabled. I am using a FlatList with pull to refresh => with refetch. When I am doing a code change, save the file and pull down I am getting TypeError: Cannot read property 'refetch'

[danieldunderfelt]

I have the same issue. Fast-refresh is getting more and more popular and I can't imagine this being a problem for only a small part of users. I tried the v3 beta, but it did not fix the problem.

[jfrolich]

The worrying part is that this must be due to assumptions made in the code that are incorrect. Fast refresh should work with any React app, so this bug is just a symptom of incorrect code (a type error/undefined check probably).

[tm1000]

The line is now: https://github.com/apollographql/apollo-client/blob/master/src/react/data/QueryData.ts#L464

[obrejla]

Hi, in which version can we expect the fix? We are facing this as well in common react web app. Thanks.

[tm1000]

@obrejla there is no 'fix'. You can follow the open PR here: #6314

[stephankaag]

The same issue exists in obsSubscribeToMore.

[danielkv]

same here

[dohomi]

I have the same issue with @apollo/client 3.1.3 and react-scripts FAST_REFRESH=true. Version 3.0.x works fine

[benjamn]

Fast refresh isn't magic, and has lots of failure modes that are not adequately documented by folks who promote its use. It sounds like some of you have been misled by deceptive marketing. See #6661 (comment) for another recent comment I made along these lines. I am happy to consider ways of keeping the magic alive as much as we can, but blind faith in flawless fast refresh with no effort is unproductive.

[tm1000]

@benjamn the "deceptive marketing” comes directly from the react team. In fact the reply to your comment that you link to states “Fast-Refresh is fully supported by React“. One of the “folks” that support its use is Dan Abramov the lead developer of react! It’s surprising that you know nothing about this and just dismiss it as a “plugin” issue to begin with while vaguely stating something about webpack hot module reload (related but also unrelated as webpack hot module reload was not supported by the react team but fast refresh is supported by the react team)

More information about the react supported fast-refresh from Dan Abramov: facebook/react#16604 (comment)

[benjamn]

@tm1000 Any resource created by a module that might be replaced needs to be cleaned up and recreated whenever the module itself is replaced, or whenever a dependency changes in a way that affects how the resource was created. If you're not writing logic in your modules to handle resource cleanup in that way (and I would guess that you are not, because almost nobody does), then your code will not be completely compatible with any automated fast refresh system. Modules that only export a single React component can be made to work in many cases, because they don't create any resources of their own, but that's a narrow/convenient subset of all possible modules. The React team has struggled ever since https://www.youtube.com/watch?v=xsSnOQynTHs to deliver on their promises without the caveats, and Fast Refresh is definitely a step forward, but only for modules written in a React-specific style. They can make promises about how React works with their refresh system, but the fundamental problems of resource management remain. I don't really want to argue with you, but I do believe this is worth understanding.

[tm1000]

My only argument here was dismissing fast-refresh as some third party module issue that is deceptively marketed by “folks” when the react team lead is pushing it out there and yes the react team can deceptively market stuff as well but the vague reference to webpack hot module reload left me wondering if you knew fast refresh was fully supported. I hear you on the difficulties within especially since Apollo isn’t react “only” it works with other SPA frameworks (and even non spas). For now when Apollo breaks during fast refresh I just refresh the page and it works again. It’s a trade off as in production it’s not an issue and all the wonderful features you’ve worked so hard on in Apollo really make up for it.

[tm1000]

Here’s the official react source code for fast-refresh https://github.com/facebook/react/tree/master/packages/react-refresh

[benjamn]

I doesn't make a difference to me that the React team is making the promises. I worked at Facebook on the JavaScript Infrastructure team in 2013-2014, when React was first open-sourced (still the 16th most prolific contributor by commits). They are doing their best to give you a great development experience, but the caveats and nuances are worth your time to understand, so that you can effectively work with/around them. Full support just means they are (bravely) taking the blame for all the things that can go wrong in a system like this. I don't envy the position they're putting themselves in.

Concretely, there's a good chance you may end up writing $RefreshReg$ and $RefreshSig$ hooks for some Apollo-specific stuff, as explained in that comment you linked above. I'm curious to hear the details of what's necessary, since this specific system is new to me as well. Maybe there's a way Apollo Client can make it easier. I'm glad that the React team is finally opening up the system and not sugar-coating what's required, but I'm not sure everyone is really listening, based on the discussion above. Hype is poisonous in large quantities.

[tm1000]

@benjamn thank you for the detailed explanation and spending some of your time writing out your thoughts about it all. I can’t disagree with what you’ve said.

[jfrolich]

Fast refresh only "refreshes" React components and does a full refresh when any other code is reloaded. Cleanup can be done in hooks (effects). It looks like the hooks work when using in "normal" situations, but they crash when using fast refresh. This is probably a bug in the hook that only presents itself when using fast refresh. Probably the architecture makes incorrect assumptions about certain guarantees in React. I am pretty sure there is a way for Apollo to work correctly (or recover gracefully from) fast refresh. And I am also pretty sure you don't need Fast Refresh internals (it is implemented differently in different bundlers), all that needed is playing well with the React effects and renders and not making incorrect assumptions (even though they hold under normal circumstances).

[pmmmwh]

Hi - maintainer of the fast refresh Webpack plugin here. I wanted to reach out and help with this specific issue, clear out some confusion, and hopefully allow more people to benefit from fast refresh.

I read the discussion here and in #6661 -

Does the fast refresh plugin have any sort of API that modules can hook into to clean up / recreate resources before/after the module is refreshed?

@benjamn No at the moment. I am not sure if we want to implement one - each integration will probably have to bring their own, so unless we have a strong reason to do so I would rather not do it (I can only speak for Webpack-related integrations though). I do understand that there is need for resource clean-up with regard to "unpure" renders.

Essentially how FR works is that it "re-renders" the component, preserving state and refs, and at last "re-mount" all effects so side-effects will be synchronised (i.e. flush + run useEffect). This resulted in a problem in Apollo's hooks (I traced via useBaseQuery), which depend heavily on the mount-unmount nature of effect before hooks to properly cleanup resources that are stored in refs. This result in an awkward flow - the update Apollo hooks received will be an amalgam of "render" and "on new data" events.

What went wrong?

I did some investigation with fast refresh while fiddling with Apollo's internals to find out what is exactly wrong. Here, I'm assuming that data should be refetched on FR to keep consistency - I think it is easier - but I'm not familiar with the code-base so please correct me (on anything below) if appropriate!

• The existence of QueryData.previousData.query, which means prevQuery === query is true. This means a second call to QueryData.removeQuerySubscription did not happen. In reality I think this did not affect the flow itself because QueryData.cleanup also calls the same function.
• isMounted from QueryData: On FR, isMounted will be true, but it probably should be false instead. This breaks the check in QueryData.execute, which created a subscription and yields an infinite loading state. (Actually, if effects flush earlier before the re-render it might have worked. I need to check with Dan on that point.)
• Cleanup functions: At the end of FR, events are flushed - which means that QueryData.cleanup and QueryData.unmount will get called again - flushing query subscriptions. Although the effects are then re-run, it broke Apollo's "assumption" on transactions (i.e. these functions should only be called when a subscription exists to be meaningful). This results in a broken subscription which will propagate downwards in any user code (hence this.currentSubscription is undefined).

I think it is quite clear that the problem definitely is not the misuse of the ! operator, lacking of the ?. operator. The proposed fixes (and linked PRs) does not address the root problem: a seemingly impossible transactional execution. The fix in my mind is a restructure of stuff in the hook. Unfortunately I am not sure how I should progress - I do not want to break Apollo's promises (e.g. performance?) for the sake of making FR work ...

Here's the raw function trace if anyone is interested.

Render

execute ->
refreshClient ->
cleanup ->
removeQuerySubscription:cleanup ->
removeQuerySubscription:execute ->
updateObservableQuery ->
initializeObservableQuery ->
refreshClient ->

getExecuteSsrResult ->
refreshClient ->
getExecuteResult ->
getQueryResult ->
startQuerySubscription ->



useEffect:refAssignment ->
afterExecute

On Fast Refresh

execute ->
refreshClient ->
cleanup ->
removeQuerySubscription:cleanup ->

updateObservableQuery ->
initializeObservableQuery ->
refreshClient ->
startQuerySubscription ->
getExecuteSsrResult ->
refreshClient ->
getExecuteResult ->
getQueryResult ->
startQuerySubscription ->
cleanup ->
removeQuerySubscription:cleanup ->
unmount ->
useEffect:refAssignment ->
afterExecute

On New Data

onNewData:next ->
execute ->
refreshClient ->



updateObservableQuery ->


startQuerySubscription ->
getExecuteSsrResult ->
refreshClient ->
getExecuteResult ->
getQueryResult ->
startQuerySubscription ->


unmount ->
afterExecute

(Empty lines are alignment for easier diffing)

[jfrolich]

@pmmmwh Great analysis! Thanks!

[gaearon]

I wanted to jump in to clarify something from the React perspective.

Editing arbitrary code inside stateful side effectful modules (like Apollo internals) is definitely not expected to work. @benjamn is correct that we can't handle arbitrary modules that create resources during initialization. This is exactly why Fast Refresh integrations would force a full reload if you edit a module that exports something other than React components.

But I'm not sure this is what's being discussed in this issue.

My impression is that in this issue we're discussing what happens when you save a file defining a React component. In this case, yes, we're making an assumption that the module itself is safe to re-execute. In other words, we assume that you're not going to performs side effects in the initialization file of a component like Button or Comment. Putting side effects into module initialization of a component is dangerous because this makes them order-dependent and you can break the code by e.g. loading this component lazily or changing the module graph in some other way. I think it is a reasonable assumption that most Apollo users would not be firing side effects in the initialization code of their component modules.

So why do we have this issue? I haven't looked at it closely because the original report doesn't actually describe what the user did. It only says which line throws. If we saw what their module code looked like and what their edit looked like, we could make a better guess. But my guess is that the error is likely related to how Hooks are being Fast Refreshed.

Here, we're not talking about random "failure modes". We're talking about guarantees and semantics that React provides. For Fast Refresh, these semantics are precisely documented here. Concretely:

• When possible, Fast Refresh attempts to preserve the state of your component between edits. In particular, useState and useRef preserve their previous values as long as you don't change their arguments or the order of the Hook calls.

• Hooks with dependencies—such as useEffect, useMemo, and useCallback—will always update during Fast Refresh. Their list of dependencies will be ignored while Fast Refresh is happening. For example, when you edit useMemo(() => x * 2, [x]) to useMemo(() => x * 10, [x]), it will re-run even though x (the dependency) has not changed. If React didn't do that, your edit wouldn't reflect on the screen!

• Sometimes, this can lead to unexpected results. For example, even a useEffect with an empty array of dependencies would still re-run once during Fast Refresh. However, writing code resilient to an occasional re-running of useEffect is a good practice even without Fast Refresh. This makes it easier for you to later introduce new dependencies to it.

My guess about this issue from the limited information it contains is that Apollo Hooks don't handle these semantics. In particular, Fast Refresh semantics are that state/refs get preserved, but all Hooks with dependencies will ignore their dependencies one time. If this is the problem, it should definitely be possible to adapt Apollo to handle these semantics.

This will also make it more compatible with other features we might add in the future (for example, row virtualization) where you might also expect dependency arrays to occasionally be "ignored" for one cycle (e.g. to represent a row appearing/disappearing). So fixing this is not just a Fast Refresh quirk but something that will be useful in the future.

[gaearon]

In other words, the TLDR is that code written in the "mount / unmount" mindset and relying hard on the [] semantics rather than "apply effect / cleanup effect" mindset may get into impossible conditions with Fast Refresh. I totally understand how this can be frustrating, but again, this will continue to be an issue for other React features coming in the future like virtualization, offscreen trees, Suspense, and so on. So Fast Refresh can serve as an early canary to find and fix them. If there is a particular pattern you're struggling to fix, please file an issue in the React repo and I will do my best to help.

[gaearon]

If you're creating resources during render it is true that they don't have a clear lifetime. This may eventually become a problem by itself (unrelated to Fast Refresh) because React generally assumes that renders are free to abort. E.g. Suspense relies on this, and so does Concurrent Mode in general. We want to be able to "throw away" incomplete trees and start from scratch, before any of the effects have fired. We don't have (or currently plan) a cleanup mechanism for something created during the render phase.

That said, you may be able to use a similar workaround to what Relay's (soon-to-be-deprecated) useLazyLoadQuery does:

https://github.com/facebook/relay/blob/6a088ae58a0d61ce81a639b2717f1d7d2403e607/packages/relay-experimental/useLazyLoadQueryNode.js#L79-L106

This is a hacky way to prevent cleanup from running until an actual "unmount" by detecting Fast Refreshes. It's not pretty but it works with the semantics.

[gaearon]

Here is how you can test something like this: facebook/relay@6f8e83d

[benjamn]

@gaearon That's helpful context for future investigation (thanks!), but we really need some sort of reproduction from the other commenters to confirm any possible diagnoses or fixes. In the absence of a reproduction, your guess is (understandably!) a bit of a leap. Running hooks more often than usual doesn't strike me as something that should cause problems for Apollo Client.

To be clear, I suspect this issue is something that can ultimately be fixed (or worked around) by Apollo Client, without changing anything about how Fast Refresh works. I will certainly let you know if I turn out to be wrong about that, but not before we know what's really going on here.

[gaearon]

My assumption is a leap but it's mostly based on @pmmmwh's analysis:

Cleanup functions: At the end of FR, events are flushed - which means that QueryData.cleanup and QueryData.unmount will get called again - flushing query subscriptions. Although the effects are then re-run, it broke Apollo's "assumption" on transactions (i.e. these functions should only be called when a subscription exists to be meaningful). This results in a broken subscription which will propagate downwards in any user code (hence this.currentSubscription is undefined).

We'd definitely need a repro to look furhter — @pmmmwh can you share one?

[jfrolich]

The description of this issue is very poor, I am sorry for that, I must have had very little time.

Editing arbitrary code inside stateful side effectful modules (like Apollo internals) is definitely not expected to work. @benjamn is correct that we can't handle arbitrary modules that create resources during initialization. This is exactly why Fast Refresh integrations would force a full reload if you edit a module that exports something other than React components.

Yes, and completely understandable. But this is indeed not what this issue is about. I do think within hooks there is enough "real estate" to have non-crashing behavior with the build-in effects (with fewer guarantees than previously expected in the apollo hooks).

Please note that this is an old issue, at that time if I recollect correctly you needed to patch the apollo package because otherwise fast refreshing any component that was using useQuery would raise an exception (which I PR'd previously with a naive patch to Apollo apollographql/react-apollo#3490). I think the bug is still there but in more subtle cases (such as the refetch function being undefined after fast-refetch). But I really have to dive in to get to reproducible cases with the latest build.

[pmmmwh]

@gaearon @benjamn Sorry that I was caught in something else for the past week - here's a reproduction. It is just a fork of the official Apollo Client setup CodeSandbox plus stuff needed to integrate with the FR plugin.

To reproduce the issue, run yarn start then change anything within the App component, like updating the header. The ExchangeRates component will be stuck in the loading state infinitely.

Edit:
Also note that this is quite specific - if the ExchangeRates component is separated into another file, I don't think the issue exist.

[tm1000]

Heres another reference I found interesting in the case of use-debounce and how they fixed their issues with fast-refresh: vercel/next.js#13268 (comment)

[ArnaudBarre]

Edit:
Also note that this is quite specific - if the ExchangeRates component is separated into another file, I don't think the issue exist.

Yeah the issue with this code is that you're creating the client in the same file as a React component. Quoting Dan:

In this case, yes, we're making an assumption that the module itself is safe to re-execute. In other words, we assume that you're not going to performs side effects in the initialization file of a component like Button or Comment

[andreialecu]

I opened #7952 which I verified to fix this issue in react-native in our own projects.

The fix is based on @gaearon's suggestions in #5870 (comment)

I put a patch-package-ready diff here: https://gist.github.com/andreialecu/49982426d4271773d9ff4935715fb2a6 for easy testing.