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

Prevent unbinding of errorHandler.report #41

Merged
merged 1 commit into from Dec 8, 2015

Conversation

lolmaus
Copy link
Contributor

@lolmaus lolmaus commented Dec 7, 2015

fixes #40

shame

@lolmaus
Copy link
Contributor Author

lolmaus commented Dec 7, 2015

That false sense of security clumsy tests give you...

@lolmaus lolmaus force-pushed the patch-1 branch 2 times, most recently from fafd680 to ff06e0f Compare December 7, 2015 16:55
@jherdman
Copy link
Owner

jherdman commented Dec 7, 2015

Would you mind squashing this down?

@lolmaus
Copy link
Contributor Author

lolmaus commented Dec 7, 2015

Nope, that's a merge and I've got no idea how to squash it.

Found some info here, but both solutions are very hacky and destructive.

If you know how to do it, I add you to my fork so that you do it yourself.

@jherdman
Copy link
Owner

jherdman commented Dec 7, 2015

The following should fix ya up: git rebase master (assuming your master branch is in sync with upstream).

@lolmaus
Copy link
Contributor Author

lolmaus commented Dec 7, 2015

The more you know... It's done.

@@ -27,6 +27,7 @@
"ember-cli-app-version": "^1.0.0",
"ember-cli-content-security-policy": "0.4.0",
"ember-cli-dependency-checker": "^1.1.0",
"ember-cli-es5-shim": "0.1.1",
Copy link
Owner

Choose a reason for hiding this comment

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

Is this merely defensive? I'm not too worried about old ass browsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for PhantomJS. This polyfill is in devDependencies and will not make it into app builds.

@jherdman
Copy link
Owner

jherdman commented Dec 7, 2015

OK, so I get what's happening here, but I'm not really sure how this bug can happen. Can you give me an example scenario?

@lolmaus
Copy link
Contributor Author

lolmaus commented Dec 8, 2015

Wut? Just throw any error, and Ember.onerror will try to invoke unbound handler.report() which references this.reporter.track but this.reporter is undefined.

jherdman added a commit that referenced this pull request Dec 8, 2015
Prevent unbinding of errorHandler.report
@jherdman jherdman merged commit 1203106 into jherdman:master Dec 8, 2015
@lolmaus
Copy link
Contributor Author

lolmaus commented Dec 8, 2015

Thank you James! 🍻

yeeees

@lolmaus
Copy link
Contributor Author

lolmaus commented Dec 8, 2015

Will you release this?

@jherdman
Copy link
Owner

jherdman commented Dec 8, 2015

Already released! 0.2.4 is out, and 1.0 is hot on its tail (same as 0.2.4, just with the 1.0 name to indicate that I believe it's production ready).

@lolmaus
Copy link
Contributor Author

lolmaus commented Dec 8, 2015

You rock! 🗿

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants