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
update react dependencies #1201
Conversation
@@ -53,7 +53,6 @@ const run = async () => { | |||
react: 'React', | |||
redux: 'Redux', | |||
axios: 'axios', | |||
recharts: 'Recharts', |
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.
we've never used it
// checking if both Database and Resource have at least isAdapterFor method | ||
// @ts-ignore | ||
if (Database.isAdapterFor && Resource.isAdapterFor) { |
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 line has been like this forever but I wonder if it's ever evaluated to false.
BaseDatabase#isAdapterFor
and BaseResource#isAdapterFor
are always defined and throw NotImplementedError
so this check should always return true. We can theoretically make BaseResource
and BaseDatabase
abstract classes or we can simply remove this "if" condition but I decided to do it another time. I added @ts-ignore
because Typescript v3 ignored this line while Typescript v4 says it always returns true.
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.
Looks fine but I don't have any experience in React 18
const isActive = useMemo(() => sortBy === property.propertyPath, [sortBy, property]) | ||
|
||
const query = new URLSearchParams(location.search) | ||
const oppositeDirection = (isActive && direction === 'asc') ? 'desc' : 'asc' |
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.
Enum for desc
and asc
would be great
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 currently used in too many places, I'll focus on rewriting this when working on AdminJS v7, otherwise Jarek and others will have more work to do when synchronizing changes while they split frontend to another repo
28c2e29
to
ee605fa
Compare
# [6.1.0](v6.0.6...v6.1.0) (2022-08-10) ### Features * update react dependencies ([#1201](#1201)) ([1205e48](1205e48))
🎉 This PR is included in version 6.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Updated React dependencies to work properly with React v18:
react-redux
to v8 - this shouldn't cause any breaking changes.react-router
andreact-router-dom
to v6. This shouldn't cause any visible changes to most of the applications based on AdminJS. However, if you're importingbuildActionClickHandler
, thepush
param has been replaced withnavigate
due touseHistory
being removed fromreact-router
:OLD:
NEW:
You can read more in the migration guide: https://reactrouter.com/docs/en/v6/upgrading/v5
react-beautiful-dnd
has been replaced withreact-dnd
. This library has been used for allowing you to change the order of array elements.react-beautiful-dnd
hasn't been updated to React 18 though and it's development is stale. There shouldn't be any breaking changes caused by this update.