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

fix(angular): Handle routes with empty path #7686

Merged
merged 2 commits into from Apr 3, 2023

Conversation

de-don
Copy link
Contributor

@de-don de-don commented Mar 31, 2023

Update function getParameterizedRouteFromSnapshot to handle routes with empty paths.

Fixes GH-7681

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • [+] If you've added code that should be tested, please add tests.
  • [+] Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Copy link
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.

Hey @de-don, thanks for opening this PR!

This fix generally LGTM so I hope we can get it in soon. Can you please tell us with which Angular version you tested this change? Since we support Angular 10 to 15, we should make sure we tested it at least in NG10 and NG15.

packages/angular/test/tracing.test.ts Outdated Show resolved Hide resolved
packages/angular/test/tracing.test.ts Outdated Show resolved Hide resolved
Update function getParameterizedRouteFromSnapshot to handle routes with empty paths.

Fixes getsentryGH-7681
@de-don
Copy link
Contributor Author

de-don commented Mar 31, 2023

Hey @de-don, thanks for opening this PR!

This fix generally LGTM so I hope we can get it in soon. Can you please tell us with which Angular version you tested this change? Since we support Angular 10 to 15, we should make sure we tested it at least in NG10 and NG15.

I have tested this code on my project with Angular 15, and as I checked, tests in repo check this code with Angular 10.

The main idea of this code, that we should proceed with path: "", but old code stops on this value. No new/old feature used in my changes.

@Lms24
Copy link
Member

Lms24 commented Apr 3, 2023

Thanks for your response @de-don! I have just tested this in NG10 and it still seems to work. Will ✅ and merge soon. Thanks again for contributing!

I have tested this code on my project with Angular 15, and as I checked, tests in repo check this code with Angular 10.

FYI, technically, that's true but the routing instrumentation is unit-tested without Angular's testbed/testing environment. It just unit-tests the getParameterizedRouteFromSnapshot function. This isn't great obviously but at the moment we don't have the resources to improve Angular tests. In the long-run, I'd like to create e2e/integration tests, like we have for NextJS or Remix for example. This way, we can just ensure that we actually send the correct stuff. Let's see when we get to it.

@Lms24 Lms24 merged commit 776d9ce into getsentry:develop Apr 3, 2023
29 checks passed
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.

Wrong transaction name for angular route event
2 participants