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

Query store is not updated during client side navigation #431

Closed
fehnomenal opened this issue Jul 21, 2022 · 8 comments
Closed

Query store is not updated during client side navigation #431

fehnomenal opened this issue Jul 21, 2022 · 8 comments
Labels
Still investigating Further information is requested

Comments

@fehnomenal
Copy link
Contributor

fehnomenal commented Jul 21, 2022

Edit: sorry this got so long, omg

This must have something to do with the store api as the behavior (or rather bug) only occurred after upgrading from before 0.15.

As you might have already noticed in my project I'm building a shop. So for context and better understanding: there are product overview pages (search results, catalogs, ...) which link to individual product pages.

I noticed the bug because each product links to related products but clicking on the links just "redirects" back to the same product. Quotes because it is not a real HTTP redirect but the client side sveltekit thing.

Ok, now my analysis:

On the product page (which lives in [id=integer]-[...wayne].svelte) I execute the query GetProductPublic($id: Int!) { productById(id: $id) { id ...ManyFragments } } which naturally queries the database for the product with the id passed in the url.

This query is an inline query so I used the afterLoad hook to check if the product exists (if (!data.GetProductPublic.product) {) and whether the passed slug is the same as in the database (if (params.wayne !== data.GetProductPublic.product.slug) {) and redirects otherwise.

I inserted some debugger; statements to debug the flow and noticed that the fetch function of queryStore decides whether to update the store:

// we want to update the store in four situations: ssr, csf, the first load of the ssr response,
// or if we got this far and the variables haven't changed (avoid prefetch)
const updateStore =
!isBrowser ||
isComponentFetch ||
(lastVariables === null && variableChange) ||
!variableChange

and this is false

  • on browser; and
  • in a route; and
  • with already set variables; and
  • changed variables

This does not seem correct? I think it really should update the store if the variables changed (which I verified they do).

Also I do not understand the reasoning why not to update in the browser?


I thought the problem is related to afterLoad not getting the latest data or the likes so I tried using the store API with an external query. load now looks like this (which should be equivalent to the load generated by the preprocessor + my afterLoad):

  export const load: Load = async (event) => {
    const { data } = await GQL_GetProductPublic.fetch({
      event,
      variables: { id: parseInt(event.params.id, 10) },
    });

    if (!data?.product) { // (1)
      return {
        status: 404,
        error: 'Wir konnten das gewünschte Produkt leider nicht finden...',
      };
    }

    if (event.params.wayne !== data.product.slug) {
      return {
        status: 301,
        redirect: productUrlPublic(data.product) + event.url.search,
      };
    }

    return {};
  };

But now even stranger things happen:

  1. Visiting a product page from the overview with an empty cache (i.e. after F5) returns a 404. This is probably due to (1) as the store is empty at the start.
  2. Going one page back and one page forward (which are client side navigations) now show the correct data. Which means the sore seems to be loaded now.
  3. Going back again and visiting a product with a different slug results in an error Redirect loop. After this client side navigation still works (going back) and going back again to the product with the loop now works... After going back and forward a few times I can reproduce this (maybe something with the cache size?).
  4. Now stranger: Going from one product to a related product does update the page contents but not the url, not even the id part... Now client side navigation still works but seems really borked and after some backs and forwards I am greeted by the redirect loop again.

I think the problem of not updating the query store remains the same...

@jycouet
Copy link
Contributor

jycouet commented Jul 21, 2022

Thanks for this deep dive 👍

I think that 2 things needs attention in your code.

1/ On the page, you need to "synchronize" fetch data. with something like:

<script context=module lang=ts>
export const load: Load = async (event) => {
const variables = { id: parseInt(event.params.id, 10) }
    const { data } = await GQL_GetProductPublic.fetch({
      event,
      variables,
    });

    //... More code ...

    return { props: {variables}};
  };
<script/>  

<script  lang=ts>
export let variables
// This is the important part. Do you have it?
$: browser && await GQL_GetProductPublic.fetch({
      variables,
});
<script/>  

Not ideal, but it's like this today. The magic of preprocessor is not here when you use "raw" stores.
We might improve it one day with: #325

2/ Client side navigation are not blocking by default

You have a Deep Dive section about it here: https://www.houdinigraphql.com/api/query/store#server-side-blocking

In your case, you want to use the data in the load, so you should probably put blocking: true to make sure the request is done before navigating.


Looking forward to reading your comments.

@AlecAivazis
Copy link
Collaborator

sorry this got so long, omg

ha! No problem at all. It's really useful to have all this info out of the way at first.

only occurred after upgrading from before 0.15.

yea that's almost certainly the case, life pre-0.15 is a lot simpler than it is now since the generated load was all that mattered then. we have a whole new categories of edge cases to deal with (see below)

Also I do not understand the reasoning why not to update in the browser?

Okay, so in order to get a feeling for that boolean expression, the important thing to notice is that a prefetch that targets a different URL powered by the same route you are on will fill the same store if we aren't super careful. To be concrete, if we are looking at /product/1 and hover over a prefetched link to product/2 we need to make sure that the store does not update with info for product 2. In order to do this, we use the fetch that happens inside of the client to keep track of which set of variables the current view holds. If a request comes into the load function that does not match the variables that the component fetch is using, we don't update the store. The variable isComponentFetch is not mean to distinguish route vs non-route components but instead to distinguish the fetch happening in context="module" from the one inside of the instance context. In your situations, isComponentFetch should be true which should make the store update.

Not sure if that'll help us debug but maybe it'll make something click? If you want to hop on a call to debug this together, lemme know

@fehnomenal
Copy link
Contributor Author

Oh wow thank you both so much.

@jycouet Both of your points were totally correct. Now everything works as expected. I guess I should read the docs more carefully instead of just skimming the "few small" changes 😁

@AlecAivazis Your explanation for the boolean makes total sense now that you reminded me of the prefetch behavior. Of course the page should not change because of a mouse hover 😂

I think this bug than only applies to inline queries as the docs say about afterLoad: "Keep in mind that if you define this hook, Houdini will have to block requests to the route in order to wait for the result.". But this does not seem to be the case.

Honestly, I will close this issue in my hopes that those damn hooks will go away soon and I'm hoping to implement #398 soonish 😂

@AlecAivazis
Copy link
Collaborator

Awesome! Glad to hear you got things settled 👍

hopes that those damn hooks will go away soon

That's the dream! I really don't them and if it wasn't for the awkward variable threading, i would have removed them in 0.15.0.

But this does not seem to be the case.

Yea I think there is still a bug here so I'm going to investigate it a bit but at least we have isolated the problem. Thanks again for the report!

@fehnomenal
Copy link
Contributor Author

Damn, I definitely spoke too soon. The redirect problem still occurs

These are the first lines of my file:

<script context="module" lang="ts">
  export const load: Load = async (event) => {
    const productId = parseInt(event.params.id, 10);

    const { data } = await GQL_GetProductPublic.fetch({
      event,
      variables: { productId },
      blocking: true,
    });

    if (!data?.product) {
      return {
        status: 404,
        error: 'Wir konnten das gewünschte Produkt leider nicht finden...',
      };
    }

    if (event.params.wayne !== data.product.slug) {
      return {
        status: 301,
        redirect: productUrlPublic(data.product) + event.url.search,
      };
    }

    return {
      props: {
        productId,
      },
    };
  };
</script>

<script lang="ts">
  // imports...

  // these two lines were new
  export let productId: number;
  $: browser && GQL_GetProductPublic.fetch({ variables: { productId } });

  // for a smaller diff
  const data = derived(GQL_GetProductPublic, ({ data }) => data);

As the redirect is only really needed because of external links I can workaround by guarding it with !browser. So right now this is not a blocker for me.

@fehnomenal fehnomenal reopened this Jul 21, 2022
@AlecAivazis
Copy link
Collaborator

AlecAivazis commented Jul 22, 2022

Hm, okay no problem. I'm sure we can figure out what's going on.

You already know all this but just so you can check my line of thinking: the Redirect Loop error sounds like a route that's redirecting back to itself. Which means for some reason productUrlPublic(data.product) is not behaving how it should. Is the data value you are getting from the fetch incorrect? On the server, every request is blocking and it always updates the store so that data you get back "should" contain the latest value from the request.

By the way, the situation in (4) seems really strange. If you ever see it again can you check the console? Something breaking before the route changes means an error in the unsubscribe which would be good to explore.

Also, was your afterLoad defined in the context="module" script? We have an integration test for the hook that passes so there's at least some reason to expect it to work. Do you by chance still have the inline version that was originally failing?

@AlecAivazis AlecAivazis added the Still investigating Further information is requested label Jul 22, 2022
@AlecAivazis
Copy link
Collaborator

@fehnomenal any update on this? I'm going to close this issue to keep things tidy but I'm happy to reopen it if we can figure out what's going on. Feel free to reach out to me on discord 👍

@fehnomenal
Copy link
Contributor Author

Sorry for the delay. This was the inline query version (only the top and with stripped query fields but that should not matter):

export const productUrlPublic = ({ id, slug }: { id: number; slug: string }): string =>
  `/artikel/${id}-${slug}`;
<script context="module" lang="ts">
  export function GetProductPublicVariables({
    params,
  }: Parameters<Load>[0]): GetProductPublic$input {
    return {
      id: parseInt(params.id, 10),
    };
  }

  export function afterLoad(
    this: RequestContext,
    { data, params, url }: Parameters<Load>[0] & GetProductPublic$afterLoad,
  ): ReturnType<Load> {
    if (!data.GetProductPublic.product) {
      return this.error(404, 'Wir konnten das gewünschte Produkt leider nicht finden...');
    }

    if (params.wayne !== data.GetProductPublic.product.slug) {
      return this.redirect(301, productUrlPublic(data.GetProductPublic.product) + url.search);
    }

    return {
      stuff: {
        activeCatalogSlug: data.GetProductPublic.product.catalogs[0]?.slug,
      },
    };
  }
</script>

<script lang="ts">
  // many imports

  const { data } = query<GetProductPublic>(graphql`
    query GetProductPublic($id: Int!) {
      product: productById(id: $id) {
        ...ProductLd
        id
        brand
        slug
        name
        ...ProductImage
        ...ProductExtraInfo
        ...ProductPrice
        ...AvailableStock
        ...AndSoOn
      }
    }
  `);
</script>

I remember that during debugging this, I inserted some console.logs in the hook function to see what is going on. I noticed that params.id was always correct (i.e. the id that was in the href of the link and in the url) but data.product.id was the last id (i.e. the id of the page where the link was placed) and data.product.slug also had the last value. In fact I logged the complete data.product object and always got the values of the current page.

Because of that my guess is that the preprocessed querying is not blocking as advertised by the docs.

I just built the version and strangely the generated load function has the blocking modifier set...
This is the relevant client code:

function afterLoad({ data, params, url }) {
  var _a;
  if (!data.GetProductPublic.product) {
    return this.error(404, "Wir konnten das gew\xFCnschte Produkt leider nicht finden...");
  }
  if (params.wayne !== data.GetProductPublic.product.slug) {
    return this.redirect(301, productUrlPublic(data.GetProductPublic.product) + url.search);
  }
  return {
    stuff: {
      activeCatalogSlug: (_a = data.GetProductPublic.product.catalogs[0]) == null ? void 0 : _a.slug
    }
  };
}
async function load(context) {
  const _houdini_context = new RequestContext(context);
  const _GetProductPublic_Input = _houdini_context.computeInput({
    "config": config,
    "framework": "kit",
    "variableFunction": GetProductPublicVariables,
    "artifact": _GetProductPublicArtifact
  });
  if (!_houdini_context.continue) {
    return _houdini_context.returnValue;
  }
  const _GetProductPublic = await store_GetProductPublicStore.fetch({
    "variables": _GetProductPublic_Input,
    "event": context,
    "blocking": true
  });
  await _houdini_context.invokeLoadHook({
    "variant": "after",
    "framework": "kit",
    "hookFn": afterLoad,
    "input": {
      "GetProductPublic": _GetProductPublic_Input
    },
    "data": {
      "GetProductPublic": _GetProductPublic.data
    }
  });
  return {
    ..._houdini_context.returnValue,
    props: {
      ..._houdini_context.returnValue.props,
      _GetProductPublic_Input
    }
  };
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Still investigating Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants