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

Show file tree
Hide file tree
Changes from all 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
15 changes: 9 additions & 6 deletions src/components/Query.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,15 @@ class QueryHandler extends Component<QueryHandlerProps, QueryHandlerState> {
this.unsubscribe = teardown;
};

state = {
executeQuery: this.executeQuery,
data: undefined,
error: undefined,
fetching: false,
};
constructor(props) {
super(props);
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 have further implemented the SSR approach but the sync would imply we execute our query here, this isn't possible since this.setState would result in a noop.

this.state = {
executeQuery: this.executeQuery,
data: undefined,
error: undefined,
fetching: props.pause !== true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This approach was suggested but isn't good for SSR as far as I understood

};
}

componentDidMount() {
this.executeQuery();
Expand Down
2 changes: 1 addition & 1 deletion src/hooks/__snapshots__/useQuery.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@ exports[`on initial useEffect initialises default state 1`] = `
Object {
"data": undefined,
"error": undefined,
"fetching": false,
"fetching": true,
}
`;
38 changes: 32 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,36 @@ 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<UseQueryState<T>>({
data: undefined,
error: undefined,
fetching: true,
});
const updateRef = 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 && !args.pause) {
const [teardown] = pipe(
client.executeQuery(request, {
requestPolicy: args.requestPolicy,
}),
subscribe(({ data, error }) => {
updateRef.current({ data, error, fetching: false });
})
);
unsubscribe.current = teardown;
}

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

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

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

useEffect(() => {
isMounted.current = true;
return () => {
isMounted.current = false;
};
}, []);

return [state, executeQuery];
};