-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
docs: async loader #1334
docs: async loader #1334
Conversation
Generally looks good, though I agree about edge cases. Would be great if we could run it through our browser test matrix to verify - maybe as part of the standard test suite? Regarding configuration: Your approach is probably already better. We could also go with a way Google Analytics handles this. Just assume a global object that has configuration. |
882b506
to
579a47c
Compare
Things to do/consider:
|
It doesn't seem to timeout anymore, so we should be fine with leaving it on TravisCI for now. I guess it's done... 'ish? |
docs/install.rst
Outdated
} | ||
}; // Don't forget the semicolon, otherwise ASI will kick-in | ||
|
||
(function(a,b,g,e,h){var k=a.SENTRY_SDK,f=function(a){f.data.push(a)};f.data=[];var l=a[e];a[e]=function(c,b,e,d,h){f({e:[].slice.call(arguments)});l&&l.apply(a,arguments)};var m=a[h];a[h]=function(c){f({p:c.reason});m&&m.apply(a,arguments)};var n=b.getElementsByTagName(g)[0];b=b.createElement(g);b.src=k.url;b.crossorigin="anonymous";b.addEventListener("load",function(){try{a[e]=l;a[h]=m;var c=f.data,b=a.Raven;b.config(k.dsn,k.options).install();var g=a[e];if(c.length)for(var d=0;d<c.length;d++)c[d].e?g.apply(b.TraceKit,c[d].e):c[d].p&&b.captureException(c[d].p)}catch(p){console.log(p)}});n.parentNode.insertBefore(b,n)})(window,document,"script","onerror","onunhandledrejection"); |
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.
I recommend putting a semicolon before the snippet to prevent people from shooting their own foot:
;(function(a,b,g,e,h){...
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.
Fair point. Changed. Unfortunately, silly prettifier removes it from the source, so I had to add a silly perl addition to the build command :S
docs/install.rst
Outdated
|
||
Or you can place those two things in a separate script tags. | ||
This will queue all errors (and promises if the environment supports ``unhandledrejection`` handler) that happened before SDK was loaded and send them once it's configured and installed. | ||
Be aware however, that there are some trade-offs to this solution, as errors might provide less information due to them being "retriggered" instead of being caught from the original source. |
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.
You could also mention that some cross-origin errors are more likely to be swallowed (and maybe link to our "what is script error" blog post).
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.
Ah, I see your comment below, but I think this can also affect production.
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.
Added
e: [].slice.call(arguments) | ||
}); | ||
|
||
if (_oldOnerror) _oldOnerror.apply(_window, arguments); |
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.
Should we be re-throwing? Or pushing these errors into Raven's internals?
If we re-throw, it means there's a possibility other onerror
instrumentation will pick up on this twice (e.g. if New Relic browser agent was loaded before ours).
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.
We are overriding onerror
handler on line 14, so it won't be picked up by anything else initially. Only when we re-trigger it, so I think it shouldn't be caught twice by any other instrumentation.
|
||
// Markers used to remove setTimeout (used for testing) from prod build, DO NOT REMOVE | ||
// build_marker | ||
setTimeout(function() { |
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.
There's a safer way to do this, at least, in Webpack, Browserify and other loaders. You can define a global variable (e.g. TEST
), that if undefined does a no-op, and have that code branch stripped during minification.
As it is, we're relying on a "DO NOT REMOVE" comment to ensure that this potentially dangerous setTimeout
never reaches production. I think we can do something a little stricter.
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.
The reason why I did it this way, was exactly to not use any webpack, browserify or other loader. As this will rarely (if ever) change, I wanted a whole thing to be a "single line" in package.json npm scripts. This way we can move it out from the repo, move it to packages, port it to v4.0 or do anything we want without remembering about any configs, loaders or similar things. Just take the source, pipe it to
cat src/loader.js | sed '/build_marker/{N;d;}' | npx google-closure-compiler-js | perl -e \"print ';'; print <STDIN>;\"
(which has no dependencies, as npx is a standard thing in npm now), and we're good to go.
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.
Okay. Just worried this might be brittle. Should probably comment explicitly what's happening here.
^bump @benvinegar |
I'm still not sure how we should approach passing CDN/DSN url and Raven configs.
As last arguments to the snippet?
As global variables with predefined names?
Also, it looks kinda big, even when compiled with closure compiler.
It's also hard to predict whether I covered all the edge cases (eg. I found out that I had to do things like on L63, as some browsers doesn't provide last argument to onerror functions).