Chore/react deprecation warnings #2022
Conversation
Release EnvironmentsThis pull request environment is provided by Release, learn more! 🔧Environment Status : https://app.releasehub.com/public/Processing%20Foundation/env-c4a0807a28 |
raclim
added
the
Create Release Environment
|
I upgraded |
There was a problem hiding this comment.
These are mostly positive changes, though it will create conflicts with some of the work that @dewanshDT has been doing and with some of my existing PRs.
I just looked at the code and I haven't loaded it up to make sure that the tabs still work as expected.
There was a problem hiding this comment.
I have an open PR that converts App to a function component but this looks right.
| type: ActionTypes.SET_PROJECTS, | ||
| projects: response.data | ||
| return (dispatch) => | ||
| new Promise((resolve) => { |
There was a problem hiding this comment.
I don't think this accomplishes anything? When we return apiClient.get().then().catch() that is already returning a Promise.
IMO we should stay away from explicit new Promise((resolve) => syntax unless it's for the purpose of converting a callback-based result to a Promise. ie. someFunction(arg, (err, res) => { if (err) { reject(err); } else { resolve(res); }}).
You could keep it as-is. You could also convert it to async/await (return async (dispatch) => {) as async functions always return a Promise.
| componentDidUpdate(prevProps) { | ||
| // if the url has changed to open a new sketch | ||
| if (this.props.params.project_id && !prevProps.params.project_id) { | ||
| if (this.props.params.project_id !== this.props.project.id) { |
There was a problem hiding this comment.
This is the same condition that was there before...but does it make sense? It's saying we should load the project if we have an id AND we did not have an id previously AND we have not already loaded the project for that id. I don't think we want the && !prevProps.params.project_id part. What if this.props.params.project_id changes from one valid id to another valid id?
| @@ -298,7 +286,9 @@ class IDEView extends React.Component { | |||
| <main className="editor-preview-container"> | |||
| <SplitPane | |||
| split="vertical" | |||
| size={this.state.sidebarSize} | |||
| size={ | |||
| this.props.ide.sidebarIsExpanded ? this.state.sidebarSize : 20 | |||
There was a problem hiding this comment.
This was actually something that I discussed with @dewanshDT in his rewrite of this component (he has it converted to a function component). We came to the same conclusion you did here -- that it's better for the state to just store the expanded size.
| } else { | ||
| url = '/projects'; | ||
| } | ||
| return apiClient |
There was a problem hiding this comment.
There's an eslint rule against using return within new Promise(). But maybe that one only showed up in the branch where I updated eslint. https://eslint.org/docs/latest/rules/no-promise-executor-return
With the previous structure, the return statement on this line causes it to return a Promise. With the current/proposed structure, the return doesn't do anything and should be deleted.
Did that actually fix the warnings? My understanding is that |
|
Some of these changes have fallen out of date, so I'm going to close this for now! |
Fixes #2021
Changes:
Removes many of the React deprecation warnings. The rest of the warnings can be removed when upgrading to React 18.
I have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123