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.

Sign up for GitHub

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

Jump to bottom

(fix) - fetching on initial render #248

Closed
wants to merge 9 commits into from
Closed

(fix) - fetching on initial render #248

Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1efab44
fetches on initial render
JoviDeCroock Jun 1, 2019
79ac12d
feat: also use it in component Query
JoviDeCroock Jun 1, 2019
ac7a89d
feat: try other implementation
JoviDeCroock Jun 1, 2019
4d03ae5
cleanup in effect
JoviDeCroock Jun 1, 2019
ae149ef
refactor: move isMounted to end of useQuery
JoviDeCroock Jun 1, 2019
0175321
fix: typo
JoviDeCroock Jun 1, 2019
1c435d3
refactor: remove redundant updateState ref
JoviDeCroock Jun 1, 2019
00b0869
feat:supply both methods
JoviDeCroock Jun 2, 2019
b030115
fix: make tests succeed
JoviDeCroock Jun 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions src/hooks/useQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,33 @@ export type UseQueryResponse<T> = [
export const useQuery = <T = any, V = object>(
args: UseQueryArgs<V>
): UseQueryResponse<T> => {
const isMounted = useRef(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoviDeCroock heads up, this is going to conflict with an existing pr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jup, saw that but in theory that invokes a will mount, no?

const unsubscribe = useRef(noop);
const initialState = useRef({ fetching: true });
const updateState = useRef(update => {
initialState.current = update;
});

const client = useContext(Context);

const request = useMemo(
() => createRequest(args.query, args.variables as any),
[args.query, args.variables]
);

const [state, setState] = useState<UseQueryState<T>>({
fetching: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simplify this PR to only change the initial state?

This line being changed from fetching: false to fetching: args.pause !== true would set the initial state as fetching by default if pause is not specified.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args.pause !== true is in theory not necessary anymore if we handle this correctly via executeQuery being either active or not active after it's run.

I suppose this PR is still in a more "experimental" stage so maybe we can merge yours for now and if that leads to conflicts I can later resolve them 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args.pause !== true is in theory not necessary anymore if we handle this correctly via executeQuery being either active or not active after it's run.

Adding this logic to executeQuery would not solve the problem of the initial rendering state though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talking about pause this should probably also be accounted in first render trying to get from cache else we’d make that option obsolete for first call.

error: undefined,
data: undefined,
});
if (!isMounted.current) {
Copy link
Member

@kitten kitten Jun 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the idea of reusing isMounted for this 👌

I suppose we could also reuse executeQuery = useCallback( /* ... */ now?

Maybe it could check whether isMounted is still false to update initialState? That would also mean that updateState becomes unnecessary.

I'm also not sure how useState works with Fiber but setState on Component was always batched, so it would only update state once. This even worked when the Component was still mounting. At least during researching the current SSR implementation I found out that useState still works similarly to that there. So if it also does in non-SSR then we could simply call setState and not worry about initialState either, since it might be batched into the first render

This would at least simplify the implementation since in summary we can:

  • Reuse executeQuery below
  • Remove updateState
  • And remove initialState potentially

Edit: Btw thanks again for picking this up! Means a lot since this is one of the more important changes and it's always great to have more people helping out 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how we can reuse executeQuery since to use that we already need our setState defined, that useState inturns relies on initialState.current.

I did implement the second remove updateState part, will ook more in depth later. Thanks for your excellent and through guidance. Amazing!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, true, reusing executeQuery definitely hinges on whether useState can again come before it and whether it's actually going to change the initialState. At least in SSR this works because calling it triggers another "loop" through the component, so not technically a rerender but a rerun. Not sure if that's the case in normal, non-SSR React

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few suggestions but if I'm not mistaken, the issue described leaves us only concerned about the initial state on mount.

It looks like we have two scenarios:

  • Pause is false and the initial state is { fetching: true }
  • Pause is true and the initial state is { fetching: false }

If that's the case, we can make this change without the need for refs or additional logic.

const [state, setState] = useState<UseQueryState<T>>({
  fetching: args.pause !== true,
  error: undefined,
  data: undefined,
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other scenario is some preparation for suspense SSR so we'll also would want to keep the executeQuery excursion that runs on initial mount outside of useEffect

But depending on how React works the executeQuery useCallback could maybe be reused for that, simplifying this PR. We'll see how that pans out I guess. Might or might nor work 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSR

I'm out of here 👀

Are you saying that following SSR, the state and ref values are not synchronized from the backend?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you what this PR becomes, my initial solution was to make the initial state based on pause. If the current wip helps out more for future work maybe we can make a seperate simple solution PR and keep this one for the experimental work.

const [teardown] = pipe(
client.executeQuery(request, {
requestPolicy: args.requestPolicy,
}),
subscribe(({ data, error }) => {
updateState.current({ data, error, isFetching: false });
})
);
unsubscribe.current = teardown;
}

const [state, setState] = useState<UseQueryState<T>>(initialState.current);

const executeQuery = useCallback(
(opts?: Partial<OperationContext>) => {
Expand Down Expand Up @@ -76,7 +90,16 @@ export const useQuery = <T = any, V = object>(
);

useEffect(() => {
executeQuery();
isMounted.current = true;
JoviDeCroock marked this conversation as resolved.
Show resolved Hide resolved
return () => {
isMounted.current = false;
};
}, []);

useEffect(() => {
if (isMounted.current) {
executeQuery();
}
return unsubscribe.current;
JoviDeCroock marked this conversation as resolved.
Show resolved Hide resolved
}, [executeQuery]);

Expand Down