Skip to content
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

Better plugin support #282

Closed
mattrobenolt opened this issue Nov 3, 2014 · 16 comments
Closed

Better plugin support #282

mattrobenolt opened this issue Nov 3, 2014 · 16 comments

Comments

@mattrobenolt
Copy link
Contributor

Need to come up with/think of a better plugin system. The current one was done as simple as possible to get things moving, but there are lots of downsides to it currently.

  • Doesn't work with AMD loaders or browserify or anything like that.
  • Raven has no concept of what's installed or how it's installed, so no way to uninstall.
  • No way to test them.
  • They rely on some magic globals.

Probably other stuff too.

@skovhus
Copy link

skovhus commented Nov 16, 2014

Let us brainstorm a bit about this... 💭

So Raven-js needs a registry of all installed plugins. And a plugin needs a common interface. It could be something like this:

  • unique name for the plugin for referencing
  • install: function(window, Raven)
  • uninstall: function(window)

Then in a Browserify scenario, it could look something like this:

var raven = require('raven-js/raven.js');
var plugin1 = require('raven-js/plugins/1');
var plugin2 = require('raven-js/plugins/1');

var options = {
    ignoreErrors: [],
    plugins: [plugin1, plugin2]
}

Raven.config('https://public@getsentry.com/1', options).install()

@mattrobenolt something like this?

@mattrobenolt
Copy link
Contributor Author

How does this work in non-browserify scenarios? Say, including as a <script> tag, or through require.js?

@skovhus
Copy link

skovhus commented Nov 16, 2014

The question is if we will continue with custom build where plugins are 'baked in'. If we do then the plugins list would already be prepopulated.

@skovhus
Copy link

skovhus commented Nov 16, 2014

Looks like your comment got removed by some XSS detector... :)

@skovhus
Copy link

skovhus commented Nov 16, 2014

Currently when including one of the custom builds with raven and some plugins, the plugins are automatically loaded, right? In the new solution we only want them to be loaded when raven is being installed.

@mattrobenolt
Copy link
Contributor Author

They are automatically loaded, but noop'd until Raven is configured/installed.

@skovhus
Copy link

skovhus commented Nov 16, 2014

Ok. So what is your take on the plugins in a non-browserify world? I guess the custom builds where everything is included is pretty nice.

@mattrobenolt
Copy link
Contributor Author

The problem is we need to covery many use cases:

  • A nice combined build suitable for a <script> tag. I personally like this since it's the most versatile and easy to get up and running. I also don't personally use these fancy package managers and whatnot.
  • Browserify
  • Normal AMD style loader
  • Installing from bower
  • Installing from npm
  • CommonJS

I understand personally 2 of these cases. <script> because lol JavaScript used to be easy, and require.js (AMD) which has been around for a long time.

@mattrobenolt
Copy link
Contributor Author

Take a look at: https://github.com/getsentry/raven-js/blob/master/template/_footer.js for what raven core needs to support. I'd imagine this all needs to carry over into plugins and needing to support somehow all of these cases as well.

@mattrobenolt
Copy link
Contributor Author

I think a good first step is providing a plugin interface that Raven understands, with the suggested install/uninstall methods. We can convert the plugins over to that. Then worry about module loading and whatnot after.

Another aspect I'm very cognizant of is file size. I try very very hard to keep raven as small as possible, and keep in mind how things will get minified, and compare gzipped sizes.

So I like to keep the public APIs small, since public APIs cannot get minified and contribute to overall size. :)

@mattrobenolt
Copy link
Contributor Author

@BYK Any opinions? Disqus has switched over to using raven.js through bower now, and through doing that has lost the plugins. I think it's bower + require.js?

So curious if there has been any thought or plans on how to make this work.

@skovhus
Copy link

skovhus commented Nov 16, 2014

Makes sense. : )

One challenge with package managers might be to get a reference to for example Backbone, as it might not be saved to window.Backbone. In some of the cases it might be necessary to pass in reference directly to the plugins. But we should be able to extend the outlined plugin system with this.

But I agree @mattrobenolt , we would start with a first plugin version where we support that plugins can be uninstalled together with Raven-js. But let's get some input on the package manager use cases.

@mattrobenolt
Copy link
Contributor Author

In the case of browserify, I found out, there literally is no window object at all. It mutates the JavaScript to actually remove any attempt at using window at compile time.

@terinjokes
Copy link

@mattrobenolt this is not true at all.

@mattrobenolt
Copy link
Contributor Author

@terinjokes Then I was misinformed. We had lots of issues checking for window.JSON because that didn't work.

See: #261

I've literally never used browserify, fwiw. I can't keep track of these things, so there is some amount of trust that people who use this are fixing bugs that affect them. :)

@benvinegar
Copy link
Contributor

We've since changed the plugins dramatically – they no-longer self-execute, can be tested, etc.

Closing this ticket.

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

No branches or pull requests

4 participants