Skip to content

Conversation

dartess
Copy link
Contributor

@dartess dartess commented Sep 16, 2019

Hello! I noticed that the location object is constantly changing. Because of this, the method for setting a new value also changes. Therefore, using the method to set a new value as deps leads to a repeated call of the effect.

@pbeshai
Copy link
Owner

pbeshai commented Sep 18, 2019

Sorry I'm not sure I entirely understand what's happening here. Can you add a test that fails with the current implementation to demonstrate the problem?

@dartess
Copy link
Contributor Author

dartess commented Sep 20, 2019

import React from 'react';
import ReactDOM from 'react-dom';
import { BrowserRouter as Router, Route } from 'react-router-dom';
import { QueryParamProvider, useQueryParam, NumberParam } from 'use-query-params';

function App() {
  const [a = 1, setA] = useQueryParam('a', NumberParam);
  const [b, setB] = React.useState(1);

  React.useEffect(() => {
    console.log('effect');
    if (b % 2 === 0) {
      setA(b);
    }
  }, [b, setA]);

  return (
    <div>
      <h4>a: {a}</h4><h4>b: {b}</h4>
      <button type='button' onClick={() => setB(b + 1)}>
        up
      </button>
    </div>
  );
}

ReactDOM.render(  
  <Router>
    <QueryParamProvider ReactRouterRoute={Route}>
      <App />
    </QueryParamProvider>
  </Router>,
  document.getElementById('root'),
);
  1. Click "up"
  2. In console:

I must indicate the setter as a dependency of the effect because otherwise I will get an lint error:

React Hook React.useEffect has a missing dependency: 'setA'. Either include it or remove the dependency array.eslint(react-hooks/exhaustive-deps)

@arianra
Copy link

arianra commented Oct 3, 2019

👍 please merge.

@offirgolan
Copy link

Any update on getting this merged and released?

pbeshai added a commit that referenced this pull request Oct 18, 2019
@pbeshai pbeshai merged commit 6cd3190 into pbeshai:master Oct 18, 2019
@pbeshai
Copy link
Owner

pbeshai commented Oct 18, 2019

Thanks for finding this and providing a fix! I ended up modifying some more memoization issues/react hook dependencies at the same time. Will publish in v0.4.2

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

Successfully merging this pull request may close these issues.

4 participants