Skip to content
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

feat(rum-react)!: support react-router 6 #1410

Merged
merged 17 commits into from
Aug 8, 2023
Merged

feat(rum-react)!: support react-router 6 #1410

merged 17 commits into from
Aug 8, 2023

Conversation

devcorpio
Copy link
Contributor

@devcorpio devcorpio commented Aug 7, 2023

Related to: #1176

Summary

This PR makes it possible to instrument applications with react-router 6.

The API we expose will look like the following snippet:

      <YourRouter>
        <ApmRoutes>
          <Route path="/home" element={<Component name="Elastic" />} />
          <Route path="/new" element={<div>new</div>} />
        </ApmRoutes>
      </YourRouter>

All the routes defined within <ApmRoutes> will be instrumented.

IMPORTANT:

This PR will not instrument the mechanism named RouteProvider and there are no plans to do so in the midterm.

Note: E2E are still pending to finish

react-router 6 doesn't support IE11. - E2E tests for React will no longer be executed for that browser.

* If the path/name is given as array, use the computed path
* to get the current transaction name
*/
function getTransactionName(name, props) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, react-router 6 doesn't allow to define array paths

}

export function getNumberOfUrlSegments(url) {
return url.split('/').filter(currentSegment => currentSegment.length > 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React-router 6 doesn't allow defining a regex as a path. Splitting by /is enough

@devcorpio devcorpio linked an issue Aug 7, 2023 that may be closed by this pull request
Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did first pass. generally looks good. Waiting for the E2E tests to finalize the review.

packages/rum-react/src/get-apm-routes.js Show resolved Hide resolved
packages/rum-react/test/specs/get-apm-routes.spec.js Outdated Show resolved Hide resolved
packages/rum-react/src/get-apm-routes.js Show resolved Hide resolved
packages/rum-react/src/get-apm-routes.js Show resolved Hide resolved
@@ -51,10 +63,13 @@ function App() {
</li>
</ul>
<Suspense fallback={<div>Loading...</div>}>
<Switch>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file even required, given there is no switch functionality in the new router, I would be fine with deleting this part of the code.

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@devcorpio devcorpio merged commit 401c9dd into main Aug 8, 2023
13 of 19 checks passed
@devcorpio devcorpio deleted the react-router-v6 branch August 8, 2023 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support react-router-dom v17
2 participants