Be explicit about side effect dependencies #366
Conversation
Add primitives as direct dependencies and establish references for non-primitives
Add primitives as direct dependencies, use references for non-primitives, and relocate function to inside side effects block
Add primitives as direct dependencies and use references for non-primitives
✅ Deploy Preview for bldrs-share ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
...and fix bad function call
|
Still WIP? Please request review when ready. |
| const location = useLocation() | ||
| const navigate = useNavigate() | ||
| const location = useRef(useLocation()) | ||
| const navigation = useRef(useNavigate()) |
There was a problem hiding this comment.
@oogali why is it better to wrap location and navigation hooks with useRef?
There was a problem hiding this comment.
When you call useEffect, you are expected to declare your dependencies: the things that should trigger when your side effect is executed. This invocation of useEffect relies on the values of the history and location objects provided by React Router.
However, if one lists useNavigate as a direct dependency, then you end up with a render loop when the underlying React code attempts to compare the value of all dependencies pre- and post-render. Keep in mind, that if any of the side effect's dependencies is a function (such as useNavigate), it will execute it and store its result for future comparisons.
The ultimate result is the initial render calls your side-effect hook, then React evaluates your dependencies which results in a call to useNavigate(), which under the hood triggers a re-render, which then triggers your side-effect hook, so on and so forth.
To avoid this, we use useRef to store a reference (or one can call it a pointer) to the history and navigation objects, and list the references as our dependencies. With this method, React is comparing the values returned by the React Router hooks rather than entering the infinite execution loop.
| @@ -157,10 +157,11 @@ export function Issues() { | |||
| debug().warn('IssuesControl#Issues: 2, no repo defined') | |||
| return | |||
| } | |||
| const fetchComments = async (selectedIssue) => { | |||
There was a problem hiding this comment.
no longer async method - why?
There was a problem hiding this comment.
This function calls getComments in src/utils/GitHub.js.
The called function blocks (via use of the await keyword) until the Github API returns a response.
Given that it is blocking, there is nothing asynchronous about this function that requires it to be tagged with the async modifier.
|
LGTM |
…deps # Conflicts: # src/Components/IssuesControl.jsx # src/Share.jsx
PR to inform React of the dependencies needed for side effects in various components. This is done through a combination of adding primitives to the dependency chains, using references (React's version of pointers) to other values, and inlining single-use methods.
This should eliminate instances of stale content (or otherwise lack of re-renders), when the underlying components change.
I did not touch
CadView.jsxyet because theonViewermethod is quite large and needs to be refactored to reduce its surface area.