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

The isValidating flag is no longer changing? #224

Closed
Svish opened this issue Jan 8, 2020 · 9 comments · Fixed by #436
Closed

The isValidating flag is no longer changing? #224

Svish opened this issue Jan 8, 2020 · 9 comments · Fixed by #436
Assignees
Labels
bug Something isn't working

Comments

@Svish
Copy link
Contributor

Svish commented Jan 8, 2020

From version 0.1.16 the isValidating flag doesn't seem to change anymore, it just stays at false, even when there is a request going on. Has the "API" of it changed after #186? Do I need to read it differently or something? Is it because I have wrapped useSWR in my own hook?

export default function useRequest(request, { initialData, ...config } = {}) {
  const { data: response, ...returned } = useSWR(
    request && JSON.stringify(request),
    () => axios(request),
    {
      ...config,
      initialData: initialData && {
        status: 200,
        statusText: 'InitialData',
        headers: {},
        data: initialData,
      },
    }
  );

  return {
    ...returned,
    data: response && response.data,
    response,
  };
}

Is there something in the way I get and pass on the returned values from useSWR here which is causing the isValidating flag not to change? If so, how would you suggest I do it instead?

Adding the following where I use my hook, just results in a single log message of undefined.

  useEffect(() => console.log(isValidating), [isValidating]);
@Svish
Copy link
Contributor Author

Svish commented Jan 8, 2020

Just tested replacing ...returned with isValidating, error, revalidate, which does make it work again. But I'm guessing this will also "undo" the re-render reduction you guys added in #186?

Is there a way I can write this useSWR wrapper so it doesn't "break" this re-render reduction?

export default function useRequest(request, { initialData, ...config } = {}) {
  const { data: response, isValidating, error, revalidate } = useSWR(
    request && JSON.stringify(request),
    () => axios(request),
    {
      ...config,
      initialData: initialData && {
        status: 200,
        statusText: 'InitialData',
        headers: {},
        data: initialData,
      },
    }
  );

  return {
    isValidating,
    error,
    revalidate,
    data: response && response.data,
    response,
  };
}

@davecoates
Copy link

This looks like it's because useSWR returns an object with getters on it... if you spread the return of that you lose the getters and the only property you'll get is revalidate.

You can keep the getters by mutating the returned object to add your own properties:

export default function useRequest(request, { initialData, ...config } = {}) {
  const returned = useSWR(
    request && JSON.stringify(request),
    () => axios(request),
    {
      ...config,
      initialData: initialData && {
        status: 200,
        statusText: 'InitialData',
        headers: {},
        data: initialData,
      },
    }
  );
  returned.response = returned.data
  return returned
}

I'm not entirely sure if there's issues with this approach though.

It is rather unexpected though - I had the same issue but I was spreading the entire response of useSWR so even data was undefined for me.

@Svish
Copy link
Contributor Author

Svish commented Jan 13, 2020

@davecoates Yeah, your workaround seem to work for me too, but not sure if the result of it is different from what I had already. And no idea if either of our solutions break/keep the optimization they've implemented with these getters. 🤔

@davecoates
Copy link

Yeah sorry for your case you'd want to add a getter on response as well so that it doesn't prematurely access data (my use case was just adding some extra options to the return that didn't need to read the values returned by useSWR)

eg. instead of returned.response = returned.data; do:

Object.defineProperties(returned, { 
    response: {
        get() { 
            return this.data;
       }
    },
});

@Svish
Copy link
Contributor Author

Svish commented Jan 14, 2020

Not sure that would work since I also replace data with response.data. I.e. I don't only add response, but replace data with something from the response 😕

What I'm trying to do here is that data should be the actual data, since that's what's expected when using the hook, but since I'm wrapping axios, then data returned from useSWR is actually the axios response object. So I need to do a little bit of a switcheroo before I return things from my useRequest wrapper. 🤔

@drews256
Copy link

Bumping this issue.

While following the steps in the readme and not wrapping the SWR request I am seeing isValidating return undefined.

Using version 0.1.16, downgrading to 0.1.15 fixes the issue.

@shuding shuding self-assigned this Jan 30, 2020
@shuding
Copy link
Member

shuding commented Jan 30, 2020

When using the spread operator, getter of the returned object can't be copied:

const { data, ...returned } = useSWR(...)

console.log(returned.isValidating) // will be undefined

Currently, we don't have a easy fix for it. I'll suggest not using ... for now:

const swr = useSWR(...)
const { data } = swr

console.log(swr.isValidating) // will change over time

@shuding
Copy link
Member

shuding commented Jan 30, 2020

The getter improvement reduces unnecessary re-renders dramatically. I'll keep this issue open and look for a better solution, or add a notice on the documentation.

@shuding shuding added the bug Something isn't working label Jan 30, 2020
@cnzh2992
Copy link

Can we add enumerable: true in defineProperties to make it present?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants