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

Create-react-app linting question #87

Closed
tobyl opened this issue Aug 29, 2019 · 15 comments
Closed

Create-react-app linting question #87

tobyl opened this issue Aug 29, 2019 · 15 comments

Comments

@tobyl
Copy link

tobyl commented Aug 29, 2019

This isn't technically an issue with use-http (which I'm currently in love with for everything fetch-related) but in create-react-app, following the basic usage example create-react-app default linting shows the error React Hook useEffect has a missing dependency: 'initializeTodos'. Either include it or remove the dependency array react-hooks/exhaustive-deps. If I follow this guidance, the error changes to The 'initializeTodos' function makes the dependencies of useEffect Hook (at line 53) change on every render. Move it inside the useEffect callback. Alternatively, wrap the 'initializeTodos' definition into its own useCallback() Hook react-hooks/exhaustive-deps.

The example seems to work exactly as expected without adding to the dependency array, but I'm wondering if this breaks best practices. Is the linter being too fussy? Or should I be modifying the example to follow this rule?

Apologies if this shouldn't be an issue, I can re-post to stackoverflow.com if recommended. Many thanks for this great module!

@alex-cory
Copy link
Collaborator

Hey! Sorry for the delay. The examples are laid out for simplicity of the beginner, but technically, you should listen to the linting rules. Wrap initializeTodos in useCallback.

@alex-cory
Copy link
Collaborator

Also, thanks so much for your support! I love this thing too! That's why I made it lol

@alex-cory
Copy link
Collaborator

@tobyl is you question answered? Gonna close this if it is :)

@tobyl
Copy link
Author

tobyl commented Sep 16, 2019

I'm sorry - I didn't get the notifications for your msgs. Yes, this is super-helpful - I'll take your advice - thank you so much!

@alex-cory
Copy link
Collaborator

alex-cory commented Sep 16, 2019

If you're having infinite loop problems, either destructure your request object and use the method as a dependency like:

const { get, loading } = useFetch(...)

const initializeTodos = useCallback(async () => {
  ...
}, [get])

or you could do this for on mount

const mounted = useRef(false)
useEffect(() => {
  if (!mounted.current) {
    initializeTodos()
    mounted.current = true
  }
})

@tobyl
Copy link
Author

tobyl commented Sep 17, 2019

Those look like two great options - I'll play around with it, thanks again!

@rhobot
Copy link

rhobot commented Mar 26, 2020

I have a related question. The lint warns that I don't add response.ok. But I believe response.ok should not be added as a dependency. Otherwise, it triggers useEffect again:

useEffect(() => {
  async function fetchSomething() {
    const data = await get(...)

    if (response.ok) {
       // ...
    }
  }

  fetchSomething()
}, [get]) // <-- eslint warns that response.ok is not added to dependency

I'm eslint-disable-line it for now. Is there a suggestion on handling this lint warning?

@alex-cory
Copy link
Collaborator

@rhobot I need to take another look. Thanks for the heads up!

@alex-cory
Copy link
Collaborator

I haven't had time to try this out yet, but based off of the NOTE portion of the use-deep-compare-effect docs I bet if we wrapped response in useMemo(() => response, []) it might not cause the infinite loop.

@rhobot
Copy link

rhobot commented Mar 27, 2020

That technically works but then eslint will warn that response is missing useMemo's dependencies.

I've also tried with uesRef but had no luck.

@alex-cory
Copy link
Collaborator

I will get to this as soon as possible. Trying to finish up retryOn and retryDelay real quick

@alex-cory
Copy link
Collaborator

@rhobot This should be working correctly in v0.4.5

@tayden
Copy link

tayden commented Jan 19, 2021

I'm still getting linter suggestions that response.ok and get should be in my useEffect dependency list. But adding response.ok to the list causes re-fetching on each render.

Is this the expected behavior? What's the correct way to deal with this?

I'm currently just excluding response.ok from the dependencies, and ignoring the lint errors.

Using use-http version 1.0.16

@alex-cory
Copy link
Collaborator

alex-cory commented Jan 19, 2021

Try passing response into useEffect

@iqoOopi
Copy link

iqoOopi commented Feb 16, 2023

Try passing response into useEffect

Hi @alex-cory , got the same question. I'm wondering why adding response to useEffect/useCallback dependency won't cause re-render?

Let's say I have three api calls inside the componet

 const { loading, response, get, put } = useFetch();
  const call1= useCallback(async () => {

    const data= await get('endpoint1');
    if (response.ok) {
         
    }
  }, [get, response]);

  const call2= useCallback(async () => {

    const data= await get('endpoint2');
    if (response.ok) {
         
    }
  }, [get, response]);

  useEffect(() => {
    call2();
  }, [call2]);

when Call1 fired, the response will be changing, thus will cause a re-render on call2 as well?

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

No branches or pull requests

5 participants