-
-
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
Window defensiveness #785
Window defensiveness #785
Conversation
The copy of TraceKit we have vendored in is already highly modified and we never can just outright update it from upstream. We hand patch in things that we care about when relevant, but we've torn out most of it to make it slimmer and better for us, so I'm not personally concerned with upstream compatibility. |
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.
Looks good, just one request. Add a comment that links to this PR so that future developers / contributors understand why we're doing this. (Sure, there's git blame, but given enough thrash that could become useless.)
Sure, done. Just to confirm, we're fine with making changes to our vendored copy of TraceKit in this way? Don't know the background on that beyond what Matt described, but sounds like it should be okay. |
Matt is 100% correct. The code is barely recognizable as TraceKit anymore (which is why we imported their entire test suite). |
* Be defensive against window being undefined * Tracekit window defensiveness * Add comments on window defensiveness
This makes us defensive against
window
not existing so we can work in webworkers. Should fix #784 and #747, though there are some things to figure out/change before merging.The reason for big 4-branch
_window
thing is that in web workers,self
is a sort-of-global-object-thing, and in Nodeglobal
is the global object (not that we need raven-js to work in node, but I was testing a few things with it as an easy env that doesn't havewindow
). If none of them exist, I fall back to an empty object rather than doing something likeFunction('return this')()
(CSP issues) or(function () { return this; })()
(strict mode issues); I'm pretty sure all of our uses of_window
still turn out okay in that case, but we won't properly instrumentsetTimeout
andsetInterval
. I'm not too worried about that case, though.I'm not sure what the best approach is gonna be to making tracekit not blow up here; obviously we don't just wanna hand edit it to be defensive every time we update our vendor copy of tracekit (like I've done in this PR just to prove things work). I don't think we can pull off any sort of slick trick where we just make the
window
reference TraceKit uses actually be something we provide like our_window
. Not sure if it makes sense for us to send them a PR to make it defensive, but I guess the webworker use case could be motivating; it looks like the current code at https://github.com/csnover/TraceKit toucheswindow
a lot more than our current version, though, and I'm not really familiar with our usage of it/interest in updating./cc @MaxBittker @benvinegar