-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fast-refresh issue #5870
Comments
I get the same error in CRA using the |
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' |
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. |
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/ |
This comment has been minimized.
This comment has been minimized.
Hi, in which version can we expect the fix? We are facing this as well in common react web app. Thanks. |
The same issue exists in obsSubscribeToMore. |
same here |
I have the same issue with @apollo/client 3.1.3 and react-scripts FAST_REFRESH=true. Version 3.0.x works fine |
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. |
@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) |
@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. |
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. |
Here’s the official react source code for fast-refresh https://github.com/facebook/react/tree/master/packages/react-refresh |
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 |
@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. |
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). |
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 -
@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 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!
I think it is quite clear that the problem definitely is not the misuse of the Here's the raw function trace if anyone is interested.Render
On Fast Refresh
On New Data
(Empty lines are alignment for easier diffing) |
@pmmmwh Great analysis! Thanks! |
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 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:
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. |
In other words, the TLDR is that code written in the "mount / unmount" mindset and relying hard on the |
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) 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. |
Here is how you can test something like this: facebook/relay@6f8e83d |
@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. |
My assumption is a leap but it's mostly based on @pmmmwh's analysis:
We'd definitely need a repro to look furhter — @pmmmwh can you share one? |
The description of this issue is very poor, I am sorry for that, I must have had very little time.
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 |
@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 Edit: |
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) |
Yeah the issue with this code is that you're creating the client in the same file as a React component. Quoting Dan:
|
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 |
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.
The text was updated successfully, but these errors were encountered: