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

consistent-function-scoping: Ignore within useEffect in React Hooks #392

Closed
garyking opened this issue Sep 25, 2019 · 7 comments · Fixed by #588
Closed

consistent-function-scoping: Ignore within useEffect in React Hooks #392

garyking opened this issue Sep 25, 2019 · 7 comments · Fixed by #588

Comments

@garyking
Copy link
Contributor

In React Hooks, I got:

useEffect(() => {
  async function getItems() {}
  
  getItems()
}, [])

And it's definitely better to keep the function in the useEffect, to group it together. But this rule recommends moving out the function, of course.

Perhaps perform a check if the function is in useEffect, to ignore it?

@MrHen
Copy link
Contributor

MrHen commented Sep 25, 2019

Hm, this is an interesting exception to the rule. Things like hooks or listeners might not have any references to the parent scope.

@futpib
Copy link
Contributor

futpib commented Sep 25, 2019

And it's definitely better to keep the function in the useEffect, to group it together.

Is it definitely better? I imagine getItems having some fetching or other global side-effect code, in which case it would be better not only to move it out of the component, but maybe even into a separate file or a module.

Can you provide a more verbose (closer to real life) example to make this clearer?

@garyking
Copy link
Contributor Author

It's recommended per the React docs, at least.

@futpib
Copy link
Contributor

futpib commented Sep 26, 2019

Now that's a different case, they have ChatAPI.subscribeToFriendStatus (which I would consider analogous to getItems from your example) defined somewhere outside the component and useEffect call, that's what I'd do (define it in a separate file). In this example handleStatusChange would not be reported as it captures setIsOnline from the component scope.

EDIT: Oh wait, I guess the rule would report handleStatusChange as it could be defined in the component scope. I don't think it's a problem, but I see what the issue is about, thanks!

import React, { useState, useEffect } from 'react';

function FriendStatus(props) {
  const [isOnline, setIsOnline] = useState(null);

  useEffect(() => {
    function handleStatusChange(status) {
      setIsOnline(status.isOnline);
    }

    ChatAPI.subscribeToFriendStatus(props.friend.id, handleStatusChange);
    // Specify how to clean up after this effect:
    return function cleanup() {
      ChatAPI.unsubscribeFromFriendStatus(props.friend.id, handleStatusChange);
    };
  });

  if (isOnline === null) {
    return 'Loading...';
  }
  return isOnline ? 'Online' : 'Offline';
}

@fisker
Copy link
Collaborator

fisker commented Feb 16, 2020

useEffect(() => {
  async function getItems() {
    function foo() {} 
  }
  
  getItems()
}, [])

What do we do with function foo?

  1. ignore
  2. move to top of useEffect
  3. move to top of file

@hatched-tom
Copy link

@fisker I believe 2 makes the most sense here, unless I'm missing something

@charliematters
Copy link

This doesn't seem to work when using the default import of react:
React.useEffect(() => { ... })

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

Successfully merging a pull request may close this issue.

7 participants