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: Fix toString usage for function wrapped using wrap method #1135

Merged
merged 1 commit into from
Nov 17, 2017

Conversation

kamilogorek
Copy link
Contributor

Fixes: #1132 (comment)

It broke, because zone.js has some code that's triggered for IE/Edge only, and it modified calls context – https://github.com/angular/zone.js/blob/dcc285aab37bdd90b6d7133fc73045100861c61c/lib/browser/event-target.ts#L67-L97

This caused all wrap calls to be passed through it, and called toString on them – https://github.com/angular/zone.js/blob/dcc285aab37bdd90b6d7133fc73045100861c61c/lib/browser/event-target.ts#L82 Because of that, function wrapped, which we return from wrap calls was given as a context to Function.prototype.toString.apply, as it also contains __raven__ flag set to true.`

The problem was, that all functions wrapped with either wrap and fill had mentioned __raven__ flags, but original function on one of them was called __inner__ and one __orig_method__. This was untraceable with tests, as it's just an odd edge case, where zone.js overrides all API methods and then core.js polyfills some of them.

Oh well... ¯_(ツ)_/¯

@kamilogorek kamilogorek requested a review from a team November 16, 2017 18:01
Copy link
Contributor

@MaxBittker MaxBittker left a comment

Choose a reason for hiding this comment

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

🏅 nice fix kamil. nasty one!

src/raven.js Outdated
@@ -367,7 +367,7 @@ Raven.prototype = {
// Signal that this function has been wrapped already
// for both debugging and to prevent it to being wrapped twice
Copy link
Contributor

Choose a reason for hiding this comment

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

we should comment that this could mean raven wrapped or filled

@kamilogorek kamilogorek merged commit 2bd838d into master Nov 17, 2017
@kamilogorek kamilogorek deleted the fix-zone-wrapping branch November 17, 2017 10:04
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.

2 participants