Skip to content
This repository was archived by the owner on Jun 10, 2019. It is now read-only.

add if statement to check if Raven object has been instantiated#958

Closed
ZacharyKearns wants to merge 1 commit into
OperationCode:masterfrom
ZacharyKearns:raven
Closed

add if statement to check if Raven object has been instantiated#958
ZacharyKearns wants to merge 1 commit into
OperationCode:masterfrom
ZacharyKearns:raven

Conversation

@ZacharyKearns

@ZacharyKearns ZacharyKearns commented Apr 24, 2018

Copy link
Copy Markdown

Description of changes

Checks to make sure that the Raven object exists before calling any methods on it.

Issue Resolved

Fixes #955

Comment thread public/index.html
return window.location.host === 'operationcode.org';
}
}).install();
if (window.Raven) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ZacharyKearns Wrapping the inline <script> tag in an if-statement prevents the Raven-related console errors, but that's because the if-statement will never be true. So this also prevents the Raven.config method from ever being called. That's because when this script tag runs, Raven hasn't been loaded because it's loaded asynchronously w/ a defer attribute on line 37 in the head (this was introduced as a performance improvement in PR #932).

Removing the defer would probably fix this (and there would be no need for the if-statement either, since Raven would have already been loaded synchronously from CDN).

@kylemh I see three options here:

  1. Remove the defer from the Raven CDN script for now (simple fix)
  2. Explore one of the more complicated ways to load Raven asynchronously (Asynchronous Loading and Capturing Errors getsentry/sentry-javascript#169 outlines some of these)
  3. Remove Raven completely - I've never used it, and I don't know of any of the current maintainers who have, either. YAGNI

Thoughts?

@jjhampton jjhampton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See inline comments - console errors are cleared, but Raven config isn't being set up w/ this solution.

@kylemh

kylemh commented May 26, 2018

Copy link
Copy Markdown
Member

Fixed in #980

@kylemh kylemh closed this May 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Raven not working on production

3 participants