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

Add upsertQueryData functionality #1720

Closed
chrisjagoda opened this issue Nov 11, 2021 · 72 comments
Closed

Add upsertQueryData functionality #1720

chrisjagoda opened this issue Nov 11, 2021 · 72 comments
Labels
enhancement New feature or request
Milestone

Comments

@chrisjagoda
Copy link

chrisjagoda commented Nov 11, 2021

I have a use case where I would like to pass an additional parameter when calling an endpoint, where I do not want this additional parameter to be cached. The first parameter, args, would still be serialized as the cache key as normal.

Would it be possible to provide an additional field to pass some additional data when calling an endpoint via a hook/initiate? My use case is for calling a query endpoint with initiate.

Perhaps an additional optional parameter after options? Something likeextraArgs, noCacheArgs, or nonCacheableArgs. I don't know if it makes sense have it on the options parameter, https://redux-toolkit.js.org/rtk-query/api/created-api/endpoints#initiate, but that would work too.

@chrisjagoda
Copy link
Author

chrisjagoda commented Nov 11, 2021

Specifically, my use case is to cache the result from a create api call in a different cache. I can do this, but it comes at the cost of having to override serializeQueryArgs, and having to have some additional logic in the providesTags field for postById.

If I could pass the response from createPost in postsClient.endpoints.postById.initiate, I could see that I have that data in the queryFn for getById, and if so, return it, caching it, and if not, call the api and cache it as normal.

import { createApi } from "@reduxjs/toolkit/query/react";
import posts from "some-api";
import { getByIdProvidesTags, getByIdQueryFn, mapClientResponse, serializeGetByIdArgs } from "./postsHelper";

export const POSTS = "posts";
export const POST_BY_ID = "postById";

const { get: getPosts, getById: getPostById, create: createPost, update: updatePost } = posts;

const postsApi = createApi({
    reducerPath: "posts",
    tagTypes: [POSTS, POST_BY_ID],
    baseQuery: ({ args, method }) => method(...args).then(mapClientResponse),
    serializeQueryArgs: serializeGetByIdArgs(POST_BY_ID),
    endpoints: (builder) => ({
        posts: builder.query({
            query: (args) => ({ args: [args], method: getPosts }),
            providesTags: (result, error, args) => [{ type: POSTS, id: JSON.stringify(args) }],
        }),
        postById: builder.query({
            queryFn: getByIdQueryFn(getPostById),
            providesTags: getByIdProvidesTags(POST_BY_ID),
        }),
        createPost: builder.mutation({
            query: (args) => ({ args: [args], method: createPost }),
            invalidatesTags: (result, error) => (error ? [] : [{ type: POSTS }]),
            onQueryStarted: (_args, { dispatch, queryFulfilled }) =>
                queryFulfilled.then(({ data }) => dispatch(postsClient.endpoints.postById.initiate(data))),
        }),
        updatePost: builder.mutation({
            query: (args) => ({ args: [args], method: updatePost }),
            invalidatesTags: (result, error) => (error ? [] : [{ type: POSTS }]),
        }),
    }),
});

export default postsApi;
export const { usePostsQuery, usePostByIdQuery, useCreatePostMutation, useUpdatePostMutation } = postsClient;

@phryneas
Copy link
Member

Without the code for getByIdQueryFn and serializeGetByIdArgs and maybe a bit more in-depth explanation, I really have no idea what exactly you are trying to accomplish here, sorry.

Generally, no, that is not supported, and so far I have not seen a use case valid enough to add first-class support for it, as it is very uncommon, there are ways around it (although clumsy) and would probably lead to lots of bugs and unexpected behaviour for users: the serialized arg is the base for the decision "do we make another request?" and many people would wonder why, even if they changed an argument, no additional request would be made.

@chrisjagoda
Copy link
Author

chrisjagoda commented Nov 19, 2021

This is what those two functions look like.

// Serialize args for getById endpoints
// Use when calling getById endpoint by either the id or the passed object to map the id from passed object to serializedKey
export const serializeGetByIdArgs =
    (getByIdEndpointName: string) =>
    ({ endpointName, queryArgs }) =>
        `${endpointName}(${
            endpointName === getByIdEndpointName && isObject(queryArgs) ? queryArgs.id : JSON.stringify(queryArgs)
        })`;

// Call getById and cache response if id passed as args, or cache getById object if object passed as args
// Use when calling getById endpoint by either the id or with an object to cache the object based off its id instead of retrieving it and then caching it
export const getByIdQueryFn =
    (method) =>
    <T>(args, _api, _extraOptions, baseQuery): MaybePromise<QueryReturnValue<T, unknown, unknown>> =>
        isObject(args) ? { data: args } : baseQuery({ args: [args], method });

// Provide id as tag id if id passed as args, or provide object id as tag id if object passed as args
// Use when calling getById endpoint by either the id or with an object to map the id from from passed object to tag
export const getByIdProvidesTags = (tagName) => (result, error, args) =>
    [{ type: tagName, id: isObject(args) ? args.id : args }];

Essentially what I'm doing with them is handling the case where instead of passing the id to postById to retrieve the post by id (the usual case), I'm passing the entire post as the arg to cache it in postById, without actually making the call to retrieve it.

If I could pass an additional parameter to an endpoint, I could avoid having to override the method serializeGetByIdArgs and not have the extra logic to handle two different types of args (the id or the post) in getByIdProvidesTags and getByIdQueryFn.

Instead in createPost I could do something like:

createPost: builder.mutation({
            query: (args) => ({ args: [args], method: createPost }),
            invalidatesTags: (result, error) => (error ? [] : [{ type: POSTS }]),
            onQueryStarted: (_args, { dispatch, queryFulfilled }) =>
                queryFulfilled.then(({ data }) => dispatch(postsClient.endpoints.postById.initiate(data.id, undefined, { data }))),
        }),

and then getByQueryFn could look like so, so that the endpoint method is only called when extraArgs.data is falsy, else we cache the data, as in my case the response returned from createPost is the same as postById.

export const getByIdQueryFn =
    (method) =>
    <T>(args, _api, _extraOptions, baseQuery, extraArgs): MaybePromise<QueryReturnValue<T, unknown, unknown>> =>
        extraArgs?.data ? { data: extraArgs.data } : baseQuery({ args: [args], method });

@chrisjagoda
Copy link
Author

I can think of some additional cases where having a place to put args that aren't part of the cache key would be useful.

One might be if you needed to have different options when calling an endpoint. Perhaps something like having a different retry or backoff approach for an endpoint when called from one place, but not another.

These options would have to be passed to the endpoint conditionally, and to have them share a cache and do so is not easy.

@phryneas
Copy link
Member

So in that specific use case... how should provides work? If invalidated, it would fire the same query and return the old value instead of fetching new, updated data from the server. Are you sure you don't just want to prime your cache with some data from another source?
In that case, cache management utilities might be more useful.

As for different backoffs: imagine, two component are mounted with the same cache entry - but different backoffs. Which would win? And if the components mounted in a different order, or the first one rerendered after the second one rendered, which one would win then? Would it cause a request every time?
Coordinating something like that is very complicated - even for built-in functionality like polling. Having it only on the endpoint is at least a bit more manageable.

@chrisjagoda
Copy link
Author

chrisjagoda commented Nov 19, 2021

Basically, there is one route to pass data to queries/mutations, that is through args. I want a backdoor to pass additional data, called extraArgs, with the difference made clear that anything passed through this backdoor will not be subject to any of the rules/side effects of args.

Provides would still behave the same, my "backdoor" cached items should behave no different than one that is normally cached.

I looked into the cache management utilities and I couldn't really find any way to provide what I want. I want the posts cached by id as args in the rtk query api endpoint, but I want to be able to freely create new cache entries, without going through the process of calling the endpoint api to retrieve the data.

Really, even more to my point and more direct/useful would be a method like

createQueryData(endpoint, args, data) // endpoint being the endpoint, same as in updateQueryData, args - the cache args, data - the data we want to cache

postsClient.util.createQueryData("postsById", data.id, { data }), that I could call instead the weirdness I'm doing calling postsClient.endpoints.postById.initiate(...), in createPost -> onQueryStarted. However, I think there may be an issue opened for something like this.

Backoffs may be more complicated on further consideration.

@phryneas
Copy link
Member

As far as I know, people have been using patchQueryData​ with a patch in the form [ { op: "replace", path: [], value: yourNewValue ] for that.

@chrisjagoda
Copy link
Author

chrisjagoda commented Nov 22, 2021

That doesn't work for my use case. patchQueryData will only update query data for a cached entry. It won't create if there's not one there.

@phryneas
Copy link
Member

Fascinating, someone was reporting they were doing this. But yeah, looking at the code, it shouldn't work.

I guess sooner or later we will need a upsertQueryData functionality. I'm not a fan of it, because it will enable a lot of patterns people could use to hurt themselves, but I guess they'll find workarounds that are even more dangerous otherwise.

@phryneas phryneas changed the title Allow passing an additional parameter that isn't cached to queries/mutations Add upsertQueryData functionality Nov 23, 2021
@phryneas phryneas added the enhancement New feature or request label Nov 23, 2021
@phryneas phryneas added this to the 1.8 milestone Nov 23, 2021
@chrisjagoda
Copy link
Author

chrisjagoda commented Nov 29, 2021

I guess they'll find workarounds that are even more dangerous otherwise.

Indeed we will, haha. Thank you for taking the time to understand my problem.

@kasparkallas
Copy link

In my RTK-Query API slice, I have single entity queries and list queries and they both return same type of objects. When I do a list query then I would like to upsert the cache for single entity queries. So this feature would help out. :)

@joel1st
Copy link

joel1st commented Aug 18, 2022

Another potential use case:
With projects like Next.js - during SSR we may want to use cached API responses for endpoints that have largely static payloads. At the moment, if we want to use RTKQuery we're kind've out of luck as there isn't a nice way to insert this data without firing off a HTTP request. This results in some SSR endpoints being much slower than they need to be.

@phryneas can you think of a way around this that wouldn't require this upsertQueryData functionality?

@markerikson
Copy link
Collaborator

This is now available in https://github.com/reduxjs/redux-toolkit/releases/tag/v1.9.0-alpha.1 . Please try it out and let us know how it works!

@davidrhoderick
Copy link

@markerikson Thanks for this release, I'm excited to try it out!

So in my situation, we are sending a cart_token, which is our cache key, to our API but that token can be either an encoded one that generates a unique token OR it can be the unique token itself. For example, if I send the API __c__generatedToken==, I'll receive {data: {cart_token: asdfghjkl12345 ...}}. Or I can send it asdfghjkl12345 and get the same data. That's why we want to de-duplicate this entry because the __c__generatedToken== is only used once at the beginning and up until now, we have been fetching from the API twice, once with the generation token and once with the unique token, just to fill the cache entry.

So here's my endpoint:

import { vrioApi } from '../vrioApi';

const extendedVrioApi = vrioApi.injectEndpoints({
  endpoints: (builder) => ({
    carts: builder.query<any, { cart_token: string }>({
      query: ({ cart_token }) => ({
        url: `carts/${cart_token}`,
        method: 'GET',
      }),
      transformResponse: (response) => ({
        ...(response as any),
        order: {
          ...(response as any).order,
          shipping_profile_id:
            (response as any).order.shipping_profile_id ?? '',
        },
      }),
      async onQueryStarted(_arguments, { dispatch, queryFulfilled }) {
        try {
          const { data } = await queryFulfilled;

          dispatch(
            extendedVrioApi.util.upsertQueryData(
              'carts',
              { cart_token: data.cart_token },
              data
            )
          );
        } catch {}
      },
    }),
  }),
  overrideExisting: true,
});

export const { useCartsQuery, useLazyCartsQuery } = extendedVrioApi;

When I run this, I get an infinite loop of the onQueryStarted function. I can see that data is correct but it just loops forever and never gets to the next function. Am I doing something wrong here? Should I not add it to the onQueryStarted callback?

@markerikson
Copy link
Collaborator

Hmm. That... actually sorta makes sense, given how we implemented upsertQueryData.

We opted to implement it as a tweak to our existing "actually start this async request" logic. That runs the entire standard request sequence, including dispatching actions...

and also the cache lifecycle methods.

You've got an onQueryStarted, in a carts endpoint, that is also upserting into a carts endpoint. So, the initial actual request is going to run onQueryStarted, which is going to call upsertQueryData, which then triggers its own onQueryStarted, which.... recurses.

I'm going to say this is probably a bug in the design and we'll have to rethink some of this behavior.

@phryneas , thoughts?

@markerikson markerikson reopened this Aug 29, 2022
@phryneas
Copy link
Member

phryneas commented Aug 29, 2022

I don't really get what you are trying to do here. The code updates the exact same cache entry with the value it already just got?

@davidrhoderick
Copy link

@phryneas it's more like the cache key we initially use is for generating the cache key that we want to use thereafter:

  1. The user hits a URL with __c__asdfghjkl12345== in the URL.
  2. This is what we use to fetch the cart so we pass it as the argument to the query.
  3. The API identifies this argument as one that generates a unique cart_token and returns data with that.
  4. The unique cart_token returned in data is what we want to continue referencing, so we also update the cart_token with the unique one when we've received.

If a user were to keep hitting the URL with __c__asdfghjkl12345== in it over and over again, they'd keep creating unique cart_tokens and they'd never be able to mutate the cart and complete the order because it's not actually tied to data.

Walking you all through this it makes me wonder, if we can identify the generation token, would it be better to use a mutation to "create" the unique token with the upsertQueryData function?

@markerikson
Copy link
Collaborator

That seems reasonable to me, yeah

@phryneas
Copy link
Member

Or at least you should check that the endpoint you want to update is not the endpoint that just has been updated - I mean, just compare the argument of the current query with the argument of the query you want to upsert.

@davidrhoderick
Copy link

davidrhoderick commented Aug 29, 2022

@phryneas that seemed like the most streamlined approach so I tried it and I could get out of the infinite loop, so thanks for that!

However, I am still hitting the API for that cart_token. Basically, when the cart_token in data differs from the one in the URL, I redirect to the one from data, which triggers a query for that one. What I'm expecting upsertQueryData to do is to add that cache entry so when I change the URL, it just fetches the cached value. Instead, it appears to be still trying to fetch it from the API i.e., I still get 2 query calls even though I should have a cache entry that I upserted already.

Edit: Working on my implementation so maybe it's something on my end.

@phryneas
Copy link
Member

Do you see onQueryStarted executing twice or two actual requests?
upsertQueryData triggers the lifecycle methods as if a request would be made, that is intended. It doesn't really make a request though, but immediately resolves with the upserted data.

@davidrhoderick
Copy link

I'm hoping my use-case isn't super specific to the point where this isn't helpful but let me flesh out the application a bit more:

First of all, before I can even fetch the data from API, I need an access_token, which I've used standard Redux state to hold since it is easily accessible from within RTK Query i.e., I wrap the axios call for a custom query function with the necessary access_token header.

Therefore, I don't make the useCartsQuery call until I have an access_token, then I use useLazyCartsQuery to trigger it when I do. Unfortunately, it'll take a bit of work to go around that check so I would like to avoid that unless you tell me this won't work with lazy queries.

After that lazy query, I compare the returned cart_token from the data with what's in the URL (this is a Next.js application so I use the useRouter hook to grab that). Then I redirect to the proper Next.js dynamic route containing the cart_token from data. For example, if I go to http://localhost:3000/__c__asdfghjkl12345, fetch the cart, and receive a new cart_token = "newCartToken", I redirect to http://localhost:3000/newCartToken. Then the application would check for an access_token, which we have from before, and immediately trigger the useLazyCartsQuery function to fetch the new data. I call that with cartsQuery({cart_token: 'newCartToken'}, true); to prefer the cached value.

So the questions are:

  1. Is using a lazy query going to work with upsertQueryData?
  2. Could the redirect and unmounting + remounting components be clearing the cache somehow?

@phryneas in response to your question, I see the onQueryStarted running twice, although in the conditional for upsertQueryData, it only runs once. On top of that, I see a second API request in the Firefox console to get the cart with the new token. That second API request shouldn't be happening if we have it in the cache and I'm preferring that value with the lazy query, correct?

@davidrhoderick
Copy link

@markerikson Thanks for your analysis. Sorry about the CRA repo, it was the end of the work day and I was sort of just trying to get something done quickly so I messed it up. In the end, I was still getting the same response even if the correct RTK version, but you saw that.

@phryneas Thanks for that fix! So basically, you are skipping the child query unless the parent query is successful, that way the timing lines up that the cache has the upserted data? Very clever (I didn't know about that skipToken import).

I do utilize this cart query all over my project but maybe I could wrap this code with a custom hook just to ensure I can render the page with the first data and in the background, make the second query.

I'll give it a try and see if it works, thanks for your help!

@agusterodin
Copy link

agusterodin commented Sep 6, 2022

Super looking forward to this functionality!

Before I create a minimal reproduction of a problem I am running into while trying this new feature out, let me know if I am potentially misusing it or misunderstanding its capabilities.

I am running 1.9.0-alpha.1

<a
  onClick={async event => {
    event.preventDefault()
    await dispatch(alarmApiUtil.upsertQueryData('getAlarmMetadataById', id, metadata))
    router.push(`alarm/${id}`)
  }}
>
Link
</a>

Background: we have all the metadata for an individual "alarm" in our app ahead of time from the "alarm list" endpoint (used for the list of cards). Ideally since we already have this data I would like to prepopulate the cache on the alarm details screen so the user never sees a loading spinner other than the situation where initial page load happens to be on an "alarm details" screen.

If you scrub frame by frame on the video below you will see that for a millisecond the pre-populated data appears. Unfortunately it immediately disappears and the loading state appears. Ideally the loading boolean would never be true and if the newly fetched data differs from the pre-populated data, the data would just be updated but without ever going into "loading state".

Is that possible with this feature? It would be super nice to have! Is this just a bug since upsertQueryData is still in feature preview?

Screen.Recording.2022-09-06.at.11.57.58.AM.mov

@phryneas
Copy link
Member

phryneas commented Sep 6, 2022

@agusterodin please try again with the CI build from #2669

@davidrhoderick
Copy link

davidrhoderick commented Sep 22, 2022

Back again. Thanks for your help so far, I've been able to de-duplicate the first fetch of my application.

The problem I'm currently having is driving me nuts in that I cannot replicate it outside of my company's application in CodeSandbox so I'm resorting to asking you if this is a known issue:

  1. First I fetch the data with a placeholder query argument.
  2. When the data comes back, we have the real query argument.
  3. That is how the data is upserted.
  4. Then we need to mutate that upserted data.
  5. The mutated response needs to update the cached value for the upserted query argument.
  6. After trying to update the upserted cache value, the data is undefined.

Is this to be expected? Do I need to de-duplicate and then run the query again and then I can mutate that entry?

Edit: Maybe a better question is how do I know when step 3 above is done?

@phryneas
Copy link
Member

@davidrhoderick without code, generally hard to tell. upserting should happen almost immediately when you call that method - maybe 1 or 2 ticks later.

@joel1st
Copy link

joel1st commented Sep 27, 2022

What is the approach we should use to take advantage of this with SSR?

Here is a dumbed down example of what I'm trying to achieve:
Screen Shot 2022-09-27 at 3 26 37 pm

It looks like upsertQueryData doesn't hydrate into the redux store during hydration (initiate call above does though):
Screen Shot 2022-09-27 at 3 28 14 pm

As a work around to the hydration problem, I tried removing the return existingData; so that initiate is called afterwards, with the expectation that the call to initiate would resolve instantly (as data should be in the cache due to the upsert), unfortunately this does not happen, and it fires off an API request regardless.

I'm guessing it has something to do with there being no subscribers at the point of upsert? Just trying to work out how to get around this without firing off an API request.

My hunch is that if I had access to the forceQueryFnSymbol, I could do something like this (note the getData function is just a helper util that unsubscribes after making the request/calls .unwrap).
Screen Shot 2022-09-27 at 4 04 36 pm

@joel1st
Copy link

joel1st commented Sep 28, 2022

I can confirm that updating the upsert implementation to 'subscribe: true' (and then unsubscribe afterwards) fixes the issue.

As a design decision, Is this something that can be made configurable? Not entirely sure what the point of upsert is if you can really only update? I guess it enables subscribers that don't make API requests.. like lazy queries? (I'm probably missing something).

Edit: - I really shouldn't have glossed over some of the thread above, looks like this PR fixes the issue I'm running into: #2669 - Hopefully we have a new version released as 1.9.0-alpha.1 does not have this fix

@markerikson
Copy link
Collaborator

FYI I should hopefully be able to put out another 1.9.0 alpha tomorrow - got some other things I want to work on before I publish it.

@markerikson
Copy link
Collaborator

Just published https://github.com/reduxjs/redux-toolkit/releases/tag/v1.9.0-alpha.2 - please try it out and let us know if upsertQueryData is working right now!

@agusterodin
Copy link

Thanks good sir! Really hyped for upsert functionality.

I tried installing alpha 2 earlier today and one of my query hooks is acting weird. One of my query hooks is returning undefined data always despite response appearing completely normal in Chrome DevTools network tab. Other than the one broken query hook, all my other query hooks work fine. This issue is completely unrelated to upsert (haven't added anything related to upsert in my code just yet).

Alpha 1 has this same weird issue of query hook not working. Downgrading to latest stable version causes everything to work perfectly again.

I will narrow it down and provide a repro tomorrow if it isn't some sort of trivial mistake.

@markerikson
Copy link
Collaborator

Hmm. We've definitely moved around a lot of internal pieces on the 1.9 branch, so it's entirely likely we borked something :)

Yeah, a repro/sandbox/replay would be very helpful!

@agusterodin
Copy link

agusterodin commented Oct 9, 2022

The issue with the query hook only occurs on a page of mine that renders the page via SSR when that is the initial page you start using the website from. This is to make it so that the site doesn't break for web crawlers .

ViewerPage.getInitialProps = wrapper.getInitialPageProps(store => async ({ query, res }) => {
  const rawImageId = query.id
  const imageId = typeof rawImageId === 'string' ? parseInt(rawImageId) : NaN
  if (typeof window === 'undefined' && !isNaN(imageId)) {
    console.log('YOU LAUNCHED DIRECTLY FROM THIS PAGE SO USE SSR!')
    store.dispatch(imageApiEndpoints.getMetadata.initiate(imageId))
    await Promise.all(api.util.getRunningOperationPromises())
    const imageMetadataResponse = imageApiEndpoints.getMetadata.select(imageId)(store.getState())
    if (res && imageMetadataResponse.isError) res.statusCode = 404
  }
})

I can confirm that the console log statement only ever gets printed when you open the web app from this particular page (in NextJS).

The query hook is still broken even if I comment all code within the getInitialPageProps function like this ->

ViewerPage.getInitialProps = wrapper.getInitialPageProps(store => async ({ query, res }) => {
  // const rawImageId = query.id
  // const imageId = typeof rawImageId === 'string' ? parseInt(rawImageId) : NaN
  // if (typeof window === 'undefined' && !isNaN(imageId)) {
  //   console.log('YOU LAUNCHED DIRECTLY FROM THIS PAGE SO USE SSR!')
  //   store.dispatch(imageApiEndpoints.getMetadata.initiate(imageId))
  //   await Promise.all(api.util.getRunningOperationPromises())
  //   const imageMetadataResponse = imageApiEndpoints.getMetadata.select(imageId)(store.getState())
  //   if (res && imageMetadataResponse.isError) res.statusCode = 404
  // }
})

If I comment out the whole thing like this, my useBlaQuery hook works perfectly again ->

// ViewerPage.getInitialProps = wrapper.getInitialPageProps(store => async ({ query, res }) => {
//   const rawImageId = query.id
//   const imageId = typeof rawImageId === 'string' ? parseInt(rawImageId) : NaN
//   if (typeof window === 'undefined' && !isNaN(imageId)) {
//     console.log('YOU LAUNCHED DIRECTLY FROM THIS PAGE SO USE SSR!')
//     store.dispatch(imageApiEndpoints.getMetadata.initiate(imageId))
//     await Promise.all(api.util.getRunningOperationPromises())
//     const imageMetadataResponse = imageApiEndpoints.getMetadata.select(imageId)(store.getState())
//     if (res && imageMetadataResponse.isError) res.statusCode = 404
//   }
// })

Does this ring any immediate bells? Can you try pasting the snippet into one of the pages of your Next projects to see if your query hooks break as well?

ViewerPage.getInitialProps = wrapper.getInitialPageProps(store => async ({ query, res }) => {
  // intentionally blank
})

I'm not ruling out that my Redux setup/configuration isn't bum. Will create a reproduction if the issue doesn't immediately jump out at you after seeing this. About to do some meal prep for the week and then i'll hop back on to create that repro.

@agusterodin
Copy link

Btw, I commented out the getInitialProps stuff and tried upsert functionality. Upsert works exactly as expected! :D

@phryneas
Copy link
Member

phryneas commented Oct 9, 2022

I'm gonna guess that if you have the getInitialProps in there, you have a hydration action and if you leave it out you don't have that hydration action 🤔
Can you validate that?

Can you take a look what exactly is modified in the state through that action?

@agusterodin
Copy link

agusterodin commented Oct 9, 2022

Yes, only when getInitialProps is present does hydration action gets dispatched. If it is not present, no hydration action gets dispatched and the hook works as expected. Setting up reproduction now

1

2

3

4

@agusterodin
Copy link

agusterodin commented Oct 10, 2022

Here is a minimal reproduction. For all I know I am not setting up Redux store properly to handle hydration. That being said, I'm surprised it worked in last stable version of RTK. I really appreciate the help!

Leaving the ImagePage.getInitialProps = wrapper.getInitialPageProps(store => async ({ query, res }) => {} line but commenting out the code inside causes the query hook not to work even when you start on the "image" page.

https://github.com/agusterodin/redux-toolkit-1.9-alpha-ssr-bug-reproduction

@phryneas
Copy link
Member

I had to do some changes to this to make it run at all for me (add node-fetch and abort-controller, change the api to reqres.in), but then it works on my system.

Could you maybe tweak this so it works on a CodeSandbox so that we really can both see the same behaviour?

@agusterodin
Copy link

Will do as soon as I get back home tonight.

Are you on a version of Node below 16? Mine works fine without the node-fetch and abort-controller dependencies.

Dumb move on my behalf not to just use a public mock API 😳

@agusterodin
Copy link

I just updated my reproduction repository to use reqres.in and pushed changes.

Unfortunately CodeSandbox isn't working properly and I can't create one (I keep getting an authorization error regardless of what I try)

Screen Shot 2022-10-11 at 6 46 34 PM


This is the behavior I am experiencing when running locally. The data doesn't fetch if I start on the index page and click the link. The data does fetch if I start on the /image page (due to SSR doing its thing).

Screen.Recording.2022-10-11.at.6.47.21.PM.mov

I am on Node v16.13.0.

@agusterodin
Copy link

Were you able to recreate the issue with my updated issue reproduction repository? Is there anything else I can do to help with debugging?

@markerikson
Copy link
Collaborator

Honestly haven't had time to look at it. My last couple evenings have been focused on the codemods for the extraReducers object syntax.

I may be able to take a look over the weekend.

@phryneas
Copy link
Member

@agusterodin You can just replace github.com with githubbox.com to get a working codeSandbox of this :)

https://codesandbox.io/s/github/agusterodin/redux-toolkit-1.9-alpha-ssr-bug-reproduction

I see the problem, investigating now.

@phryneas
Copy link
Member

phryneas commented Oct 14, 2022

@agusterodin the problem here is a timing problem in combination with that hydrating reducer you are using. This works:

const reducer = (state, action) => {
  if (action.type === HYDRATE) {
+   const { api: _ignore_and_let_RTK_handle_this, ...hydrate } = action.payload;
    const nextState = {
      ...state,
-      ...action.payload
+     ...hydrate
    }
    if (typeof window !== 'undefined' && state?.router) {
      nextState.router = state.router
    }
    return rootReducer(nextState, action)
  } else {
    return rootReducer(state, action)
  }
}

Generally, it is safer to have RTK Query do the rehydration for the api slice itself.

In this specific case, there is nothing to hydrate from the server. So that reducer overwrites the client state with {}. Unfortunately, at that point the client has already started a query - and that data gets lost.

I believe this might have something to do with a timing change that seems to have happened in a recent version of next-redux-wrapper: kirill-konshin/next-redux-wrapper#493

@phryneas
Copy link
Member

phryneas commented Oct 14, 2022

Yeah... reading next-redux-wrapper@7.0.5...8.0.0, they essentially moved hydration from creation of the Root Component into a useEffect running after the first render of the App component.
That's kinda necessary since they changed from

export default wrapper.withRedux(App)

to

function App({Component, ...rest}){
    const {store, props} = wrapper.useWrappedStore(rest);
    return (
        <Provider store={store}>
            <Component {...props.pageProps} />
        </Provider>
    );
}

and they cannot really do that before first render. (wrapper.withRedux now also does that same thing)

That said, it is highly problematic. Before that change, even @agusterodin's "overwrite everything" rehydrate reducer would not be a problem. After it... well, it breaks.

@markerikson
Copy link
Collaborator

My takeaway from those last couple comments is that whatever @agusterodin was seeing isn't actually related to upsertQueryData, but rather next-redux-wrapper?

Given that, I'd like to close this issue for bookkeeping purposes. If there is anything else going on, let's open up a new issue from here.

1.9-beta.0 is ready to go - I'm just going to wait until morning to publish and announce it.

@agusterodin
Copy link

My takeaway from those last couple comments is that whatever @agusterodin was seeing isn't actually related to upsertQueryData, but rather next-redux-wrapper?

Given that, I'd like to close this issue for bookkeeping purposes. If there is anything else going on, let's open up a new issue from here.

1.9-beta.0 is ready to go - I'm just going to wait until morning to publish and announce it.

Yes, my previously mentioned issue is 100% related to the next wrapper and ended up having nothing to do with RTK 1.9. Downgrading to wrapper v7 fixes the issue.

Upsert behavior works exactly as expected 🎉 Thanks for the incredible work!

@markerikson
Copy link
Collaborator

Suh-WEEEEET!

Thank you for the fast response and for trying that out for us!

@davidrhoderick
Copy link

@phryneas @markerikson Just wanted to say thanks a lot for release 1.9.0! It looks like it's working great on my application. I had to do a couple things to get it working but they were necessary for my application:

  1. @phryneas solution for checking whether the cart_token matches data.cart_token was important or I get a forever loop. In my latest implementation, I forgot that at first and remembered the solution from before so it was necessary. Might be good to put that in documentation if it's not there.
  2. In my application, I had a parent component calling a lazy query to get the data. Then in child components, I used a non-lazy query to get the data. This actually caused me issues with a double-submit CSRF token & cookie where mid-flight requests would 403 because the CSRF token & cookie would change on SSR between redirects. I used useQueryState instead of useQuery in those child components to defeat that issue and it reminded me to come back to de-duplicating the initial call. Turns out, that's what was needed.

Plus the fact that you've now officially released this code as version 1.9.0, I have to say great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants