-
Notifications
You must be signed in to change notification settings - Fork 21
Upgrade react-router-dom to v5.0 #237
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #237 +/- ##
=========================================
+ Coverage 79.81% 80.61% +0.8%
=========================================
Files 10 10
Lines 218 227 +9
Branches 48 51 +3
=========================================
+ Hits 174 183 +9
Misses 30 30
Partials 14 14
Continue to review full report at Codecov.
|
@@ -9,6 +9,9 @@ | |||
import * as React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
|
|||
// $FlowFixMe |
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.
Can you describe what this $FlowFixMe is fixing? I usually add the error from Flow next to the $FlowFixMe. You can also just reply to this if you don't feel like adding that info to the code, because I'm interested in why we need it.
class Lifecycle extends React.Component<{ | ||
onConstruct?: () => void, | ||
onMount?: () => void, | ||
}> { |
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.
Can you assign the object type to a type variable that describes what it represents?
React.Component<{
onConstruct?: () => void,
onMount?: () => void,
}>
// changes to
type MyCoolType = {
onConstruct?: () => void,
onMount?: () => void,
};
React.Component<MyCoolType>
Is there an ETA for merging this branch into master? |
react-router 5 switches from the legacy context API to React.createContext, which breaks our usage of reading history from context in our Redirect component.
This PR is a bit of a stopgap, in the future, we should probably reconsider how router context is used and if/how/what we fork/extend/wrap from react-router. Ideally we can avoid using any private APIs and avoid similar problems in the future.