Skip to content

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

Merged
merged 3 commits into from
Apr 16, 2013

Conversation

heynemann
Copy link
Contributor

We are loading RavenJS asynchronously, which means we don't know when it will be loaded.

That makes it very hard to call config and install reliably.

This patch creates the following use case:

window.RavenCallback = function(raven) {
    raven.config(myDSNUrl).install();
};

// code to load raven asynchronously

This will be a great addition to an already great library.

@mattrobenolt
Copy link
Contributor

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.

@guilhermef
Copy link
Contributor

Hey @mattrobenolt, it's ok like this ?

@heynemann
Copy link
Contributor Author

I think both are equally important.

Consider the scenario where I want to load user data into Raven:

window.RavenCallback = function(raven) {
raven.config(dsn).install();

myAuthFramework.userDataLoaded(function(userData) {
    Raven.setUser({
        email: userData.email,
        id: userData.id
    })
});

};

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.

@heynemann
Copy link
Contributor Author

Is there anything we can do to improve this pull request?

@mattrobenolt
Copy link
Contributor

I still really hate the idea of a callback. I'd much rather use an event.

@heynemann
Copy link
Contributor Author

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.

@mattrobenolt
Copy link
Contributor

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.

@heynemann
Copy link
Contributor Author

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?

@mattrobenolt
Copy link
Contributor

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.

@heynemann
Copy link
Contributor Author

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.

@mattrobenolt
Copy link
Contributor

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.

mattrobenolt added a commit that referenced this pull request Apr 16, 2013
Included a RavenCallback global that allows loaded callbacks
@mattrobenolt mattrobenolt merged commit 10eee4e into getsentry:master Apr 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants