Skip to content

fix(vue): Correct span name assignment for matched routes in VueRouter#12398

Merged
Lms24 merged 3 commits intogetsentry:developfrom
soch4n:soch4n-vue-fix-transaction-name
Jun 7, 2024
Merged

fix(vue): Correct span name assignment for matched routes in VueRouter#12398
Lms24 merged 3 commits intogetsentry:developfrom
soch4n:soch4n-vue-fix-transaction-name

Conversation

@soch4n
Copy link
Copy Markdown
Contributor

@soch4n soch4n commented Jun 7, 2024

Fixed an issue where the correct transactionName could not be obtained when using nested routes in VueRouter.

Nested Routes Example

const routes: Array<RouteRecordRaw> = [
  {
    path: '/',
    component: () => import('@/components/Home.vue'),
    children: [
      {
        path: 'categories',
        component: () => import('@/components/Categories.vue'),
        children: [
          {
            path: ':categoryId',
            component: () => import('@/components/CategoryDetail.vue'),
          },
        ],
      },
    ],
  },
];

Before

TransactionName would always be "/" for nestedRoute.

After

The correct value (/categories/:categoryId) is set.

Reference

https://router.vuejs.org/guide/essentials/nested-routes.html

@soch4n soch4n marked this pull request as ready for review June 7, 2024 05:51
Lms24
Lms24 reviewed Jun 7, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Lms24 Lms24 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @soch4n thanks for opening this PR! The changes look good to me on first glance.

Have you tested this with Vue 3 and/or Vue 2? Similarly, does this change work with all Vue router versions we support (2,3,4) ? I'm not saying you necessarily have to test in all of these combinations but it would be great to know where this was tested.

Similarly, I think it'd be best if we could Add an E2E test for nested routes so that we don't only rely on artificially created routing events in the unit tests.
If you take a look at our Vue E2E test, I think it should be fairly straight forward to add a nested route and a test similar to this one. Are you interested in adding this test? If not, just let me know then we'll take over.

@Lms24
Copy link
Copy Markdown
Member

Lms24 commented Jun 7, 2024

Oh also, it seems like CI isn't happy with file formatting. It's easiest to run yarn fix:biome in the root of the repository. This should take care of the formatting issue.

@soch4n
Copy link
Copy Markdown
Contributor Author

soch4n commented Jun 7, 2024 •
edited
Loading

Hey @soch4n thanks for opening this PR! The changes look good to me on first glance.

Have you tested this with Vue 3 and/or Vue 2? Similarly, does this change work with all Vue router versions we support (2,3,4) ? I'm not saying you necessarily have to test in all of these combinations but it would be great to know where this was tested.

Similarly, I think it'd be best if we could Add an E2E test for nested routes so that we don't only rely on artificially created routing events in the unit tests. If you take a look at our Vue E2E test, I think it should be fairly straight forward to add a nested route and a test similar to this one. Are you interested in adding this test? If not, just let me know then we'll take over.

@Lms24
Thank you for your helpful feedback!

  • Vue 3: Vue Router v4
  • Vue 2: Vue Router v3, v2

I have verified that to.matched behaves similarly across these versions.

I've added an E2E test. 1319288
It is my first time doing this, so I am not very confident. Please check and let me know if there are any issues.

@soch4n
Copy link
Copy Markdown
Contributor Author

soch4n commented Jun 7, 2024 •
edited
Loading

Oh also, it seems like CI isn't happy with file formatting. It's easiest to run yarn fix:biome in the root of the repository. This should take care of the formatting issue.

@Lms24
I tried running yarn lint in the packages/vue directory, but it didn't work.
However, when I ran yarn fix:biome from the project root, it worked perfectly.
Thank you for the guidance!

Lms24
Lms24 approved these changes Jun 7, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for adding the test @soch4n! It looks fine to me. Let's see if CI is happy but from my PoV we can merge this and include it in the next release.

@Lms24
Copy link
Copy Markdown
Member

Lms24 commented Jun 7, 2024 •
edited
Loading

Argh right, I forgot, E2E tests don't run in CI for external PRs. I'll clone your branch and submit a draft PR to run e2e tests. Will keep you posted.

#12415

@Lms24 Lms24 mentioned this pull request Jun 7, 2024
Closed
@Lms24
Copy link
Copy Markdown
Member

Lms24 commented Jun 7, 2024 •
edited
Loading

Tests passed in #12415:

image

Great work, thanks for contributing @soch4n!

@Lms24 Lms24 merged commit 1df9c69 into getsentry:develop Jun 7, 2024
@Lms24 Lms24 added the Package: vue Issues related to the Sentry Vue SDK label Jun 7, 2024
@soch4n soch4n deleted the soch4n-vue-fix-transaction-name branch June 7, 2024 23:37
@Lms24 Lms24 mentioned this pull request Jun 10, 2024
Merged
billyvg pushed a commit that referenced this pull request Jun 10, 2024
@soch4n @billyvg
fix(vue): Handle span name assignment for nested routes in VueRouter (#…
c346948 
…12398)

Fix an issue where the correct transactionName could not be obtained
when using nested routes in VueRouter.
This was referenced Jul 3, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lms24 Lms24 Lms24 approved these changes

Assignees

No one assigned

Labels

Package: vue Issues related to the Sentry Vue SDK

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@soch4n @Lms24