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

Implement console.assert wrapping for console plugin. #654

Closed
wants to merge 2 commits into from

Conversation

frewsxcv
Copy link

@frewsxcv frewsxcv commented Jul 15, 2016

Fixes #653.


This change is Reviewable

@frewsxcv
Copy link
Author

Disclaimer: I've done minimal testing with this and haven't tested on older browsers.

@dcramer
Copy link
Member

dcramer commented Jul 24, 2016

@getsentry/javascript

@benvinegar
Copy link
Contributor

@frewsxcv – are you actually using console.assert?

My concern with this patch is that it creates console.assert if it doesn't already exist.

@frewsxcv
Copy link
Author

@frewsxcv – are you actually using console.assert?

Yes. It's specified by the WHATWG Console Spec. It's also available in all modern browsers (including IE11).

@frewsxcv
Copy link
Author

Would you prefer I check if it already exists at the global level before creating it?

@benvinegar
Copy link
Contributor

I understand that it's specified, but I'm curious if you're actually using this live, and if so, how.

BTW, any idea why MDN is saying this at the top?

This feature is non-standard and is not on a standards track ...

@frewsxcv
Copy link
Author

frewsxcv commented Jul 27, 2016

I understand that it's specified, but I'm curious if you're actually using this live, and if so, how.

For example:

var $someElement = $(".element-class");
console.assert($someElement.length);  // make sure the selector is valid

or

$.get(url, function (data) {
    var name = data.name;
    console.assert(_.isString(name) && name.length);
    ...
});

@frewsxcv
Copy link
Author

BTW, any idea why MDN is saying this at the top?

It looks like that header was added in 2014 when the initial write of that wiki page happened, which presumably predates the WHATWG standard.

@benvinegar
Copy link
Contributor

Would you prefer I check if it already exists at the global level before creating it?

Yes.

I'd also prefer if you created a method, wrapAssert inside of src/console.js so that later I can re-use it inside our breadcrumb collecting code (I won't ask you to implement that part). It will also be easier to test.

Thanks.

@benvinegar
Copy link
Contributor

It looks like that header was added in 2014 when the initial write of that wiki page happened, which presumably predates the WHATWG standard.

Ah, yeah, I'm super lazy and really should have just looked at that :) But I'd encourage you to submit an edit to this page citing the addition to the spec.

@frewsxcv
Copy link
Author

I'd also prefer if you created a method, wrapAssert inside of src/console.js so that later I can re-use it inside our breadcrumb collecting code (I won't ask you to implement that part). It will also be easier to test.

Latest commit has these changes.

@benvinegar
Copy link
Contributor

benvinegar commented Jul 27, 2016

Okay, so, to be good internet citizens, wrapAssert needs to call the original console.assert. This is in case another 3rd-party script might also be using console.assert, i.e. another logger.

Secondly, this PR also requires some kind of test. Take a look at test/plugins/console.test.js.

Sorry for the asks – this is the pain of developing for Raven ^_^

@frewsxcv
Copy link
Author

Sorry, I don't think I can spend any more time looking into this. Feel free to close this if you don't either.

@benvinegar
Copy link
Contributor

Yep, totally understandable. I'll pick it up at some point.

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

Successfully merging this pull request may close these issues.

4 participants