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(ember): keep route hook context when performance-wrapping #3274

Merged

Conversation

alexlafroscia
Copy link
Contributor

@alexlafroscia alexlafroscia commented Feb 22, 2021

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).

While integrating the Route Performance Tracking feature of @sentry/ember into my app, I ended up hitting a bunch of errors. It turns out that when wrapping the different Route functions that are performance-tracked, the context of the function is lost. This means that accessing this inside those methods -- a very common Ember pattern for accessing injected services -- breaks.

The existing tests don't cover this case because none of the routes in the dummy app that have the instrumentation wrapping access the context of the route.

I started by adding the unit test in this PR, which failed, then updated the instrumentation method definition to make things pass again 🎉

With this change the context of these methods are preserved.

@alexlafroscia
Copy link
Contributor Author

cc: @k-fish -- maybe you could take a look at this? Since I believe you have some context on the Ember integration.

Copy link
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

Yeah that looks good, and thanks for adding tests the dummy app doesn't cover. 👍

@alexlafroscia
Copy link
Contributor Author

Wonderful! I'm excited to see this land so I can use these better insights in our Ember app

@k-fish k-fish merged commit a40a0da into getsentry:master Mar 9, 2021
This was referenced Mar 11, 2021
This was referenced Mar 15, 2021
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.

None yet

2 participants