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

Bug: eslint-plugin-react-hooks cannot use optional chaining in deps #18819

Closed
coolkev opened this issue May 4, 2020 · 5 comments
Closed

Bug: eslint-plugin-react-hooks cannot use optional chaining in deps #18819

coolkev opened this issue May 4, 2020 · 5 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@coolkev
Copy link
Contributor

coolkev commented May 4, 2020

React version: 16.3.1
eslint-plugin-react-hooks version: 4.0.0

Steps To Reproduce

Use ?. in dependency to any of the react hooks.

export default function App() {
    const x = Date.now() % 2 === 0 ? { test: true } : undefined;
  
    React.useEffect(() => {
      if (x?.test) {
        console.log('test');
      }
    }, [x?.test]);
  
    return (
      <div className="App">
        <h1>Hello CodeSandbox</h1>
      </div>
    );
  }
  

Link to code example:
I was unable to reproduce the behavior in codesandbox, it must be transforming the code or stripping out the ? before linting. It happens in VS Code.

The current behavior

eslint errors occur:
React Hook React.useEffect has a missing dependency: 'x'. Either include it or remove the dependency array.eslintreact-hooks/exhaustive-deps
React Hook React.useEffect has a complex expression in the dependency array. Extract it to a separate variable so it can be statically checked.eslintreact-hooks/exhaustive-deps
image

The expected behavior

I would expect it to work just like it does currently for non-optional child member access.
image
When ? is removed, it no longer shows eslint errors, but obviously would fail at runtime if x is undefined.

@coolkev coolkev added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label May 4, 2020
coolkev added a commit to coolkev/react that referenced this issue May 4, 2020
@blydiet
Copy link

blydiet commented May 4, 2020

Hey one, Idea is inside of x you can make it look more like this
const x = Date.now() % 2 === 0 ? { test: true } : { test: undefined };
that should get the result you want I think.

Also, if you're on Firefox and you did not set privacy.reduceTimerPrecision you will not get the right reduced time precision for the date.now() the numbers will be a little random

check out the docs https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/now

@coolkev
Copy link
Contributor Author

coolkev commented May 4, 2020

The example code was just to show a simple way to reproduce the problem of the hook reporting incorrectly. The data used in my dependency checks is actually from redux store and will contain undefined objects. I use the useEffect hook to handle fetching the data if it is not present, and it can happen at multiple levels.

For example:

const OrderDetail = ({ orderId }) => {

    const order = useSelector(state => state.orders[orderId]);

    const customer = useSelector(state => order && state.customers[order.customerId]);

    const dispatch = useDispatch();

    React.useEffect(() => {
        dispatch(fetchOrder(orderId));
    }, [orderId]);

    React.useEffect(() => {
        if (order?.customerId) {
            dispatch(fetchCustomer(order?.customerId));
        }
    }, [order?.customerId])

    return <div>
        <div>Order #{order?.id}</div>
        <div>Customer: {customer?.name}</div>
    </div>
}

Obviously, this can be solved by extracting customerId to a separate const. But this again is a simple example, and I can have much more complicated dependencies that would involve extracting a bunch of throw away consts that are only used inside of the hook. Ideally, the deps checks would work just like they do currently whether or not optional chaining is used.

I've already posted a PR that fixes the issue.

gaearon pushed a commit that referenced this issue May 5, 2020
…8819) (#18820)

* eslint-plugin-react-hooks: allow OptionalMemberExpression in deps (#18819)

* add test case for #18819

* fix test

* run prettier
@gaearon
Copy link
Collaborator

gaearon commented Jul 7, 2020

Optional chaining problems should be fixed in [email protected]. If you experience some problem with optional chaining after 4.0.6, please file a new issue. Thanks.

@gaearon
Copy link
Collaborator

gaearon commented Sep 10, 2020

[email protected] should have the fix for the ESLint parser.

@vvscode
Copy link

vvscode commented Oct 4, 2020

I still have this problem with eslint-plugin-react-hooks@^4.1.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

4 participants