replace match.params with useParams hook #1837
Conversation
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
Codecov ReportAll modified lines are covered by tests ✅
📢 Thoughts on this report? Let us know!. |
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
| @@ -309,4 +310,4 @@ function mapDispatchToProps(dispatch) { | |||
| }; | |||
| } | |||
|
|
|||
| export default connect(mapStateToProps, mapDispatchToProps)(SearchTracePageImpl); | |||
| export default withRouteProps(connect(mapStateToProps, mapDispatchToProps)(SearchTracePageImpl)); | |||
There was a problem hiding this comment.
Curious, why wasn't this change needed in the previous PR where withRouteProps was introduced? Does it work both ways?
There was a problem hiding this comment.
No, it doesn't work both ways.
In the previous PR's code, it was working fine. I have just verified with the code on the main branch.
This file uses: this.props.history.push and even in the previous PR, I didn't see it using withRouter.
So, where did its history prop come from in the main branch? That's strange.
There was a problem hiding this comment.
You can too verify this by clicking on a trace spot from this chart:
In the main branch's code, it is working.
But in this PR's code, without the use of withRouteProps, this doesn't seem to work. I couldn't figure out why.
There was a problem hiding this comment.
Just figured it out.
Actually, it is due to the change in how we are using Route children and Route component.
| mapStateToProps, | ||
| mapDispatchToProps | ||
| )(TraceDiffImpl); | ||
| export default withRouteProps( |
There was a problem hiding this comment.
could you please add documentation to this function (should've asked in the prev PR)?
There was a problem hiding this comment.
Documentation in the form of jsDocs comments added throughout the function? Or is there any dedicated documentation guide for each function used in Jaeger-UI?
| @@ -46,7 +46,7 @@ type TDispatchProps = { | |||
|
|
|||
| type TOwnProps = { | |||
| history: RouterHistory; | |||
| match: match<TDiffRouteParams>; | |||
| params: TDiffRouteParams; | |||
There was a problem hiding this comment.
Does this change have any impart on the shape of the URL? IIUC the state is included in the URL.
There was a problem hiding this comment.
No. Absolutely not.
Previous URL structure:
http://localhost:5173/trace/4e9e0a8990106fdd...196659de1e174bd4?cohort=4e9e0a8990106fdd&cohort=196659de1e174bd4
Current:
http://localhost:5173/trace/51c935040996787d...0f7faf874d731c2c?cohort=51c935040996787d&cohort=0f7faf874d731c2c
It's just the place from where we use the extracted URL parameters.
| <Route path={searchPath}> | ||
| <SearchTracePage /> | ||
| </Route> |
There was a problem hiding this comment.
With this change, the child components, SearchTracePage in this example, would no longer receive the Route props.
So, we need to use the withRouteProps HOC to get them.
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>
Which problem is this PR solving?
Description of the changes
matchprop. Since the introduction of hooks, we need to useuseParams()hook for accessing that.<Route children ..instead of `<Route component ..> since it would be deprecated in v6.How was this change tested?
screen-capture.4.webm
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test