-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(ember): keep route hook context when performance-wrapping #3274
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
Conversation
cc: @k-fish -- maybe you could take a look at this? Since I believe you have some context on the Ember integration. |
There was a problem hiding this 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. 👍
Wonderful! I'm excited to see this land so I can use these better insights in our Ember app |
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
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 accessingthis
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.