-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix(react): remove console errors #385
Conversation
() => | ||
controller.subscribe(() => | ||
process.nextTick(() => setState(controller.state)) | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixing the following error:
Cannot update a component ("PagerRenderer") while rendering a different component ("ResultListRenderer")
In all the documentation I found, it says that the error can be prevented by enclosing the setState
method inside the useEffect
hook (which is already the case here).
So, is it possible that the Pager and ResultList controllers are "dispatching" their new state at the same time, which causes the 2 components to trigger a page render at the same moment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My React is rusty but... process.nextTick ? in a browser ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it is supported but I'll switch to setTimeout(0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it is supported but I'll switch to setTimeout(0).
But... How 😕 When you break on this line, what is going on 🤯 what is process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an implementation of process for browsers
https://www.npmjs.com/package/process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh why not then, curious where does it come from?
@@ -1,8 +1,8 @@ | |||
import red from '@material-ui/core/colors/red'; | |||
import {createMuiTheme} from '@material-ui/core/styles'; | |||
import {createTheme} from '@material-ui/core/styles'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createMuiTheme
is deprecated
@@ -25,7 +25,7 @@ const SearchPage: React.FunctionComponent<ISearchPageProps> = (props) => { | |||
return ( | |||
<EngineProvider value={engine}> | |||
<Container maxWidth="lg"> | |||
<Grid container justify="center"> | |||
<Grid container justifyContent="center"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
justify
is deprecated
@@ -62,16 +67,16 @@ const Hero: React.FunctionComponent<IHeroProps> = (props) => { | |||
Essential Links | |||
</Typography> | |||
<ul> | |||
<li> | |||
<li key="item-link-headless"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each item in a list should have a key
{props.logos.map((logo, idx) => ( | ||
<img | ||
src={logo} | ||
key={'image-' + idx} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each item in a list should have a key
"display": "standalone", | ||
"theme_color": "#000000", | ||
"background_color": "#ffffff" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix the following error
Manifest: Line: 1, column: 1, Syntax error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really confused by the process.nextTick
.
Other changes looks a-ok to me.
Proposed changes
Fix all the issues causing an error in the browser console
Testing
CDX-513