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

[wip] fix(useQuery): use setOptions for updating query #134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trojanowski
Copy link
Owner

@trojanowski trojanowski commented Apr 18, 2019

Thanks to that previous value of data is shown while loading new one and networkStatus should have a correct value in that case

BREAKING CHANGE: previous value of data is shown while loading new one

  • fix suspense mode
  • add tests

Closes #117
Closes #121
Closes #129

…hat previous value of `data` is shown while loading new one and `networkStatus` should have a correct value in that case

BREAKING CHANGE: previous value of `data` is shown while loading new one

Closes #117
Closes #121
Closes #129
@trojanowski
Copy link
Owner Author

WIP - unfortunately suspense mode doesn't work with that version

@FezVrasta
Copy link

Thanks so much for this!!

@fbartho
Copy link

fbartho commented Apr 18, 2019

I'm super excited about showing stale data while refreshing.

@FezVrasta
Copy link

@trojanowski can we do something to help you with this PR? I could add tests if you agree

@trojanowski
Copy link
Owner Author

@trojanowski can we do something to help you with this PR? I could add tests if you agree

@FezVrasta it would be nice, thanks. Right now the suspense mode is also broken - I'll try to fix (in the next week I think).

@FezVrasta
Copy link

FezVrasta commented Apr 23, 2019

@trojanowski I tried to add a test for #117 but I can still reproduce the bug.

You can try my branch here

@trojanowski trojanowski mentioned this pull request Apr 23, 2019
3 tasks
@trojanowski
Copy link
Owner Author

@FezVrasta yes, it seems this PR requires further work. We should implement merging previous data with the last one. react-apollo implements it that way: https://github.com/apollographql/react-apollo/blob/master/src/Query.tsx#L474. Strange that your codesandbox demo from #117 worked for me with this PR in the current form.

@FezVrasta
Copy link

@trojanowski is this still on your radar? I'm sorry for being so pushy but I would really love to use this project in production :-(

@jjenzz
Copy link

jjenzz commented May 7, 2019

@FezVrasta in case it helps, we're currently working around the data reset issue with the following:

import { useRef, useEffect } from 'react';
import { useQuery as useReactApolloHooksQuery } from 'react-apollo-hooks';

export function useQuery(query, options) {
	const prevDataRef = useRef();
	const { loading, error, data } = useReactApolloHooksQuery(query, options);
	const hasData = data && Object.keys(data).length;
	const activeData = hasData ? data : prevDataRef.current;

	useEffect(() => {
		if (hasData) {
			prevDataRef.current = data;
		}
	});

	return {
		data: activeData,
		loading,
		error,
	};
}

@zhammer
Copy link

zhammer commented May 7, 2019

@jjenzz i had thought that updating a ref doesn't trigger a rerender. is the idea here that something else (like error) updating triggers the rerender, then you just use the new value of prevDataRef.current?

(this gets deeper into my hooks confusion, so apologies if that doesn't totally make sense)

@jjenzz
Copy link

jjenzz commented May 7, 2019

@zhammer you are correct that a ref doesn’t trigger a rerender. The code above keeps a reference to successfully retrieved data so that if the data becomes empty in a future render it can return that cached data instead. Does that help?

@jtomaszewski
Copy link

@trojanowski any idea if we can anyhow go around the Suspense issue? Any idea why it doesn't work on this PR?

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.

Missing setVariables network status data set to {} between re-fetches
6 participants