-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Included a RavenCallback global that allows loaded callbacks #96
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
Conversation
What about just declaring some global options? Like: var raven_dsn = '...';
var raven_config = {}; Then when raven is loaded, it'll check those first and install itself if they exist. This is similar to how Disqus and Google Analytics and other libraries work. |
Hey @mattrobenolt, it's ok like this ? |
I think both are equally important. Consider the scenario where I want to load user data into Raven: window.RavenCallback = function(raven) {
}; That's just one of the possibilities. I'm just saying there's stuff you can't anticipate, so a callback is definitely a way to add a nice degree of extensibility to raven js. |
Is there anything we can do to improve this pull request? |
I still really hate the idea of a callback. I'd much rather use an event. |
How do you propose that works? How do I subscribe to the event? I'm willing to change this pull request to get it approved. The callback we are sending is actually an onLoaded event in disguise. Would you rather call it RavenOnLoad? We can definitely change that. |
You just dispatch a global event. Almost identical to the callback. See #84. I just wanted to clean that up and make it a little shorter before pulling it in. :) But that's the idea. I can just add this event with the other ones. |
Did you check this updated pull request? It uses a global config, the same you asked for. I still think having the event is very useful. Maybe have both? |
oh, haha, derp. Sorry about that. Then the only thing, why the changes in console.js? That's literally not even being used yet and definitely irrelevant to this PR. |
Running the tests were giving warnings ;( We thought useful to improve the codebase. If you think that's not the case we can definitely revert that. |
Yeah, I should ignore the /plugins directory from jshint. That's new stuff I'm planning. Can you just ignore it for this, and I'll get the jshint stuff. I like to keep commits relatively isolated. |
Included a RavenCallback global that allows loaded callbacks
We are loading RavenJS asynchronously, which means we don't know when it will be loaded.
That makes it very hard to call
config
andinstall
reliably.This patch creates the following use case:
This will be a great addition to an already great library.