(fix) - fetching on initial render #248

JoviDeCroock · 2019-06-01T12:12:55Z

Fixes: #245

After the initial render the effect gets executed that sets fetching to true
So when initially rendering we see that fetching is false implying we have data, since after this we'll always see fetching -> data fetching -> data

Only on initial render we see a scenario where: fetching false but no data -> fetching -> data

Made this as a draft because I understand the why but maybe we could resolve it differently with a loaded property or something?

Snapshot:


exports[`on initial useEffect initialises default state 1`] = '
Object {
  "data": undefined,
  "error": undefined,
  "fetching": false,
}
';

Fails atm due to the initialState change

kitten · 2019-06-01T12:15:38Z

@JoviDeCroock Cheers for opening a PR! 🎉 We'll have to do some other fixes though but I think that could be solved in combination with #218.

The reason is that we possibly want the initial state to match up with what the cache exchange gives us back synchronously. So depending on the cache it can also make sense to set fetching to false and have a result synchronously, so we'll have to probably solve this in combination with the small suspense rewrite. Currently since we're only triggering an exchange inside useEffect on the initial render we might not see the correct state reflected immediately.

JoviDeCroock · 2019-06-01T12:18:00Z

@kitten agreed, do you want me to try and work on #218 I have time for this tomorrow. Can understand that this is more of a author type of decision though but if I can help out i'd gladly do so

kitten · 2019-06-01T12:21:02Z

@JoviDeCroock If you'd like to help out with that that'd be amazing! ❤️ I don't have much time this weekend myself but would otherwise take a look at that next week.

Essentially we can use the fact that the exchange pipeline should run synchronously. So on an initial run of the hook we could try to get the initial state with executeQuery. So if we get a synchronous result from it, which we should sometimes do from the cache, we can then immediately use that as the initial state. Otherwise we'd set fetching to true.

So far so good I hope 😓

This would then allow me to add suspense support to the client later on, since we're running the initial state query synchronously, which means I can easily add some more code to throw a promise in the client.

JoviDeCroock · 2019-06-01T12:45:24Z

I find it hard to determine the fact that your client.execute would return synchronously isn't the point of that subscribe to not have to worry about that? Sorry if I'm totally missing the point.

kitten · 2019-06-01T12:46:45Z

@JoviDeCroock It doesn't return anything synchronously, but if there isn't anything concurrent going on in the exchange chain it'll step through each operator synchronously. So the callback in subscribe might be called before anything else happens; at least that's what's supposed to happen when the cache already has a result

JoviDeCroock · 2019-06-01T12:48:30Z

Yes, I can understand that and then we aren't fetching but as far as I know we can't be aware of a sync return in the first render since the setting of the state happens asynchronously anyway. I'll try and think a bit more in depth about this, don't want to bother you about these things if you don't have a lot of time

kitten · 2019-06-01T12:53:41Z

@JoviDeCroock It should in theory work, since we're only dealing with initial mount, i.e. in some pseudo-code-ish fashion

const useQuery = (/* ... */) => {
  const isInitialRender = useRef(true);
  const initialState = useRef({ fetching: true });
  const updateState = useRef(update => { initialState.current = update; });
  const teardown = useRef(null);

  if (isInitialRender.current) {
    isInitialRender.current = false;
    teardown.current = pipe(
      client.executeQuery(/* ... */),
      subscribe(result => updateState.current(result))
    );
  }

  const [state, setState] = useState(initialState.current);
  updateState.current = setState;
  
  useEffect(() => {
    // this is where the old updating logic goes with request.key diffing etc
  }, [/* ... */]);

  // ...
};

Can probably be simplified but I can totally see a way to make this work so that on initial mount we get the correct initial state 🤔

src/hooks/useQuery.ts

JoviDeCroock · 2019-06-01T13:36:30Z

Implemented this for useQuery, will try to find a way to test this asap and extend it to Query component as well.

kitten · 2019-06-01T13:41:48Z

src/hooks/useQuery.ts

-    error: undefined,
-    data: undefined,
-  });
+  if (!isMounted.current) {


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 🙏

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!

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

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, });

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 😅

SSR

I'm out of here 👀

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

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.

andyrichardson · 2019-06-01T13:59:24Z

src/hooks/useQuery.ts

@@ -33,19 +33,34 @@ export type UseQueryResponse<T> = [
 export const useQuery = <T = any, V = object>(
  args: UseQueryArgs<V>
 ): UseQueryResponse<T> => {
+  const isMounted = useRef(false);


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

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

andyrichardson · 2019-06-01T14:03:37Z

src/hooks/useQuery.ts

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

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


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.

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 🤔

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?

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.

JoviDeCroock · 2019-06-02T10:21:23Z

src/components/Query.tsx

-    fetching: false,
-  };
+  constructor(props) {
+    super(props);


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.

JoviDeCroock · 2019-06-02T10:21:39Z

src/components/Query.tsx

+      executeQuery: this.executeQuery,
+      data: undefined,
+      error: undefined,
+      fetching: props.pause !== true,


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

kitten · 2019-06-03T11:30:17Z

@JoviDeCroock So I tested out my hypothesis and it works 🎉 setState will synchronously update the initial state on initial mount before running any effects that will ensure that the user only sees the initial state that we want them to see. So just by running executeQuery. I definitely want to credit your work so would you like to review this and copy some of the changes over? Tests are WIP #250

JoviDeCroock · 2019-06-03T14:31:34Z

@kitten Will do, thanks for the response I'll try and check your PR tonight. What code would you need there? Closing this PR so we can keep a good overview.

Learned something new today thanks!

kitten · 2019-06-03T14:33:18Z

@JoviDeCroock However you'd like to help 🙌 I also don't have tests for it yet... Mostly, I wouldn't have landed on that implementation this quickly without your help so at the very least I'll add you as a co-author to the merge commit. But let me know what you think about it and whether you think it'll cover everything we've talked about. That'd be super helpful. Cheers! ❤️

fetches on initial render

1efab44

feat: also use it in component Query

79ac12d

JoviDeCroock added 2 commits June 1, 2019 15:29

feat: try other implementation

ac7a89d

cleanup in effect

4d03ae5

JoviDeCroock commented Jun 1, 2019

View reviewed changes

src/hooks/useQuery.ts Outdated Show resolved Hide resolved

src/hooks/useQuery.ts Show resolved Hide resolved

JoviDeCroock added 2 commits June 1, 2019 15:38

refactor: move isMounted to end of useQuery

ae149ef

fix: typo

0175321

kitten reviewed Jun 1, 2019

View reviewed changes

refactor: remove redundant updateState ref

1c435d3

andyrichardson reviewed Jun 1, 2019

View reviewed changes

JoviDeCroock added 2 commits June 2, 2019 12:06

feat:supply both methods

00b0869

fix: make tests succeed

b030115

JoviDeCroock commented Jun 2, 2019

View reviewed changes

JoviDeCroock closed this Jun 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(fix) - fetching on initial render #248

(fix) - fetching on initial render #248

JoviDeCroock commented Jun 1, 2019 •

edited

Loading

kitten commented Jun 1, 2019

JoviDeCroock commented Jun 1, 2019 •

edited

Loading

kitten commented Jun 1, 2019

JoviDeCroock commented Jun 1, 2019

kitten commented Jun 1, 2019

JoviDeCroock commented Jun 1, 2019

kitten commented Jun 1, 2019 •

edited

Loading

JoviDeCroock commented Jun 1, 2019

kitten Jun 1, 2019 •

edited

Loading

JoviDeCroock Jun 1, 2019

kitten Jun 1, 2019

andyrichardson Jun 1, 2019

kitten Jun 1, 2019

andyrichardson Jun 1, 2019

JoviDeCroock Jun 1, 2019

andyrichardson Jun 1, 2019

JoviDeCroock Jun 1, 2019

andyrichardson Jun 1, 2019

kitten Jun 1, 2019

andyrichardson Jun 1, 2019

JoviDeCroock Jun 1, 2019

JoviDeCroock Jun 2, 2019

JoviDeCroock Jun 2, 2019

kitten commented Jun 3, 2019

JoviDeCroock commented Jun 3, 2019 •

edited

Loading

kitten commented Jun 3, 2019

(fix) - fetching on initial render #248

(fix) - fetching on initial render #248

Conversation

JoviDeCroock commented Jun 1, 2019 • edited Loading

kitten commented Jun 1, 2019

JoviDeCroock commented Jun 1, 2019 • edited Loading

kitten commented Jun 1, 2019

JoviDeCroock commented Jun 1, 2019

kitten commented Jun 1, 2019

JoviDeCroock commented Jun 1, 2019

kitten commented Jun 1, 2019 • edited Loading

JoviDeCroock commented Jun 1, 2019

kitten Jun 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitten commented Jun 3, 2019

JoviDeCroock commented Jun 3, 2019 • edited Loading

kitten commented Jun 3, 2019

JoviDeCroock commented Jun 1, 2019 •

edited

Loading

JoviDeCroock commented Jun 1, 2019 •

edited

Loading

kitten commented Jun 1, 2019 •

edited

Loading

kitten Jun 1, 2019 •

edited

Loading

JoviDeCroock commented Jun 3, 2019 •

edited

Loading