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.

Sign up for GitHub

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

Jump to bottom

Be explicit about side effect dependencies #366

Merged
merged 13 commits into from
Sep 15, 2022
Merged

Be explicit about side effect dependencies #366

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e7182a5
Establish explicit dependencies for BaseRoutes side effects
oogali Aug 31, 2022
8b071da
Establish explicit dependency chain for Share side effects
oogali Aug 31, 2022
51963bf
Add Zustand store setter to Share side effects dependency chain
oogali Aug 31, 2022
8666a05
Remove redundant reference to target issue via ID
oogali Aug 31, 2022
ce215a9
Remove unnecessary async qualifier
oogali Aug 31, 2022
dbcebf8
Remove unused component property
oogali Aug 31, 2022
3089cda
Simplify selected issue check
oogali Aug 31, 2022
dfa7e9e
Update list of dependencies for Issues
oogali Aug 31, 2022
dc96058
Establish explicit dependency chain for SearchBar component
oogali Aug 31, 2022
54744ab
Re-enable exhaustive dependency checking lint rule
oogali Aug 31, 2022
d80f2d5
Update function signature to aid IDE autocompletion
oogali Aug 31, 2022
ee29ef4
Merge remote-tracking branch 'origin/main' into explicit-side-effect-…
oo-bldrs Sep 14, 2022
8b2426a
Remove improper import
oo-bldrs Sep 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 9 additions & 16 deletions src/BaseRoutes.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
import React, {useEffect} from 'react'
import {
Outlet,
Routes,
Route,
useLocation,
useNavigate,
} from 'react-router-dom'
import React, {useEffect, useRef} from 'react'
import {Outlet, Route, Routes, useLocation, useNavigate} from 'react-router-dom'
import ShareRoutes from './ShareRoutes'
import debug from './utils/debug'

Expand All @@ -26,20 +20,19 @@ import debug from './utils/debug'
* @return {Object}
*/
export default function BaseRoutes({testElt = null}) {
const location = useLocation()
const navigate = useNavigate()
const location = useRef(useLocation())
const navigation = useRef(useNavigate())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oogali why is it better to wrap location and navigation hooks with useRef?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh that makes sense!

const installPrefix = window.location.pathname.startsWith('/Share') ? '/Share' : ''
const basePath = `${installPrefix }/`

useEffect(() => {
if (location.pathname === installPrefix ||
location.pathname === (`${installPrefix }/`)) {
if (location.current.pathname === installPrefix ||
location.current.pathname === basePath) {
debug().log('BaseRoutes#useEffect[], forwarding to: ', `${installPrefix }/share`)
navigate(`${installPrefix }/share`)
navigation.current(`${installPrefix }/share`)
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [])
}, [basePath, installPrefix, location, navigation])

const basePath = `${installPrefix }/`
return (
<Routes>
<Route path={basePath} element={<Outlet/>}>
Expand Down
18 changes: 9 additions & 9 deletions src/Components/IssuesControl.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,11 @@ export function Issues() {
debug().warn('IssuesControl#Issues: 2, no repo defined')
return
}
const fetchComments = async (selectedIssue) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer async method - why?

Copy link
Contributor Author

@oogali oogali Sep 8, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const fetchComments = ((selectedIssue) => {
try {
const commentsArr = []
const commentsData = await getComments(repository, selectedIssue.number)
const commentsData = getComments(repository, selectedIssue.number)
if (commentsData) {
commentsData.map((comment) => {
commentsArr.push({
Expand All @@ -178,18 +179,17 @@ export function Issues() {
} catch {
debug().log('failed to fetch comments')
}
}
if (selectedIssueId !== null) {
fetchComments(filteredIssue)
}
})

// This address bug #314 by clearing selected issue when new model is loaded
if (!filteredIssue) {
if (filteredIssue !== null) {
fetchComments(filteredIssue)
} else {
setSelectedIssueId(null)
}
// this useEffect runs everytime issues are fetched to enable fetching the comments when the platform is open
// using the link
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [selectedIssueId, issues, repository])
}, [filteredIssue, repository, setComments, setSelectedIssueId])


return (
Expand Down
13 changes: 6 additions & 7 deletions src/Components/SearchBar.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ import TreeIcon from '../assets/2D_Icons/Tree.svg'
* @return {Object} The SearchBar react component
*/
export default function SearchBar({onClickMenuCb, showNavPanel}) {
const location = useLocation()
const navigate = useNavigate()
const location = useRef(useLocation())
const navigation = useRef(useNavigate())
const [searchParams, setSearchParams] = useSearchParams()
const [inputText, setInputText] = useState('')
const [error, setError] = useState('')
Expand All @@ -50,11 +50,10 @@ export default function SearchBar({onClickMenuCb, showNavPanel}) {
setInputText(newInputText)
}
} else {
navigate(location.pathname)
navigation.current(location.current.pathname)
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [searchParams])
}, [inputText, searchParams])

const onSubmit = (event) => {
// Prevent form event bubbling and causing page reload.
Expand All @@ -66,7 +65,7 @@ export default function SearchBar({onClickMenuCb, showNavPanel}) {
if (looksLikeLink(inputText)) {
try {
const modelPath = githubUrlOrPathToSharePath(inputText)
navigate(modelPath, {replace: true})
navigation.current(modelPath, {replace: true})
} catch (e) {
console.error(e)
setError(`Please enter a valid url. Click on the LINK icon to learn more.`)
Expand All @@ -77,7 +76,7 @@ export default function SearchBar({onClickMenuCb, showNavPanel}) {
// Searches from SearchBar clear current URL's IFC path.
if (containsIfcPath(location)) {
const newPath = stripIfcPathFromLocation(location)
navigate({
navigation.current({
pathname: newPath,
search: `?q=${inputText}`,
})
Expand Down
5 changes: 2 additions & 3 deletions src/Components/SideDrawer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export function SideDrawer({
closeDrawer,
isCommentsOn,
isPropertiesOn,
setSelectedIssueId}) {
}) {
const classes = useStyles({
divider: (isCommentsOn && isPropertiesOn),
isCommentsOn: isCommentsOn,
Expand Down Expand Up @@ -66,7 +66,7 @@ export function SideDrawer({
</div>
<div className={classes.divider}/>
<div className={classes.containerProperties}>
{isPropertiesOn ? <PropertiesPanel/> : null }
{isPropertiesOn ? <PropertiesPanel/> : null}
</div>
</div>
</Drawer>
Expand Down Expand Up @@ -113,7 +113,6 @@ export default function SideDrawerWrapper() {
isCommentsOn={isCommentsOn}
isPropertiesOn={isPropertiesOn}
openDrawer={openDrawer}
setSelectedIssueId={setSelectedIssueId}
/>}
</>
)
Expand Down
43 changes: 21 additions & 22 deletions src/Share.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, {useEffect, useState} from 'react'
import {useNavigate, useParams} from 'react-router-dom'
import React, {useEffect, useRef, useState} from 'react'
// eslint-disable-next-line no-unused-vars
import {NavigateFunction, useNavigate, useParams} from 'react-router-dom'
import {ThemeProvider} from '@mui/material/styles'
import CssBaseline from '@mui/material/CssBaseline'
import CadView from './Containers/CadView'
Expand All @@ -23,7 +24,7 @@ import AccountCircle from '@mui/icons-material/AccountCircle'
* @return {Object} The Share react component.
*/
export default function Share({installPrefix, appPrefix, pathPrefix}) {
const navigate = useNavigate()
const navigation = useRef(useNavigate())
const urlParams = useParams()
const [modelPath, setModelPath] = useState(null)
const setRepository = useStore((state) => state.setRepository)
Expand All @@ -38,7 +39,22 @@ export default function Share({installPrefix, appPrefix, pathPrefix}) {
* path, so no other useEffect is triggered.
*/
useEffect(() => {
/** A demux to help forward to the index file, load a new model or do nothing. */
const onChangeUrlParams = (() => {
const mp = getModelPath(installPrefix, pathPrefix, urlParams)
if (mp === null) {
navToDefault(navigation.current, appPrefix)
return
}
if (modelPath === null ||
(modelPath.filepath && modelPath.filepath !== mp.filepath) ||
(modelPath.gitpath && modelPath.gitpath !== mp.gitpath)) {
setModelPath(mp)
debug().log('Share#onChangeUrlParams: new model path: ', mp)
}
})
onChangeUrlParams()

// TODO(pablo): currently expect these to both be defined.
const {org, repo} = urlParams
if (org && repo) {
Expand All @@ -50,24 +66,7 @@ export default function Share({installPrefix, appPrefix, pathPrefix}) {
} else {
console.warn('No repository set for project!', pathPrefix)
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [urlParams])


/** A demux to help forward to the index file, load a new model or do nothing. */
function onChangeUrlParams() {
const mp = getModelPath(installPrefix, pathPrefix, urlParams)
if (mp === null) {
navToDefault(navigate, appPrefix)
return
}
if (modelPath === null ||
(modelPath.filepath && modelPath.filepath !== mp.filepath) ||
(modelPath.gitpath && modelPath.gitpath !== mp.gitpath)) {
setModelPath(mp)
debug().log('Share#onChangeUrlParams: new model path: ', mp)
}
}
}, [appPrefix, installPrefix, modelPath, pathPrefix, setRepository, urlParams])


const {theme, colorMode} = useTheme()
Expand All @@ -91,7 +90,7 @@ export default function Share({installPrefix, appPrefix, pathPrefix}) {

/**
* Navigate to index.ifc with nice camera setting.
* @param {Object} navigate
* @param {NavigateFunction} navigate
* @param {string} appPrefix
*/
export function navToDefault(navigate, appPrefix) {
Expand Down