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

Added CommonJS support #259

Closed
wants to merge 1 commit into from

Conversation

christophercliff
Copy link

Verified in the node repl (e.g. var Raven = require('./dist/raven')).

Not covered by automated testing--I couldn't figure out a testing approach given the current design.

Fixes #197.

@mattrobenolt
Copy link
Contributor

So this does change behavior though. window.Raven no longer exists in the AMD loader path. This went back and forth a couple times and settled on it needing to be that way. I'll dig up the related tickets.

@christophercliff christophercliff changed the title Added a CommonJS support Added CommonJS support Sep 11, 2014
@christophercliff
Copy link
Author

I assumed that was a mistake. Why leak the global when you're using a module system?

@mattrobenolt
Copy link
Contributor

Just check out the commit history for this file and you'll see the back and forth:

https://github.com/getsentry/raven-js/commits/master/template/_footer.js

You can remove the global yourself easily with Raven.noConflict().

@mattrobenolt
Copy link
Contributor

Specifcally 190d9e6 is the commit that brought it back to being a global, to support plugins easier.

@mattrobenolt
Copy link
Contributor

@christophercliff Did you close this on purpose?

@christophercliff
Copy link
Author

Yeah, I suspect this isn't the best way forward.

@mattrobenolt
Copy link
Contributor

How so? I'll take the module.exports = Raven approach. I just didn't want to remove the global.

@triblondon
Copy link

Could you consider adopting the change in https://github.com/getsentry/raven-js/pull/206/files#diff-a4cb6d8063a6aea4bba2d6f10472b5f4L7 as well, otherwise it won't work with browserify.

@christophercliff
Copy link
Author

If the global must stay, I'd be happy if you left it as-is and published every release to npm.

Maybe a crazy idea: rewrite as CommonJS and use a build tool like webpack to generate the UMD-compatible files for distribution. You could avoid some of these manual compatibility woes.

@mattrobenolt
Copy link
Contributor

Like I've said, I'm open to somebody proposing a better solution. I don't use any of these tools personally so it's a bit foreign to me. --
Sent from Mailbox

On Fri, Sep 12, 2014 at 2:28 PM, Christopher Cliff
notifications@github.com wrote:

If the global must stay, I'd be happy if you left it as-is and just published every release to npm.

Maybe a crazy idea: rewrite as CommonJS and use a build tool like webpack to generate the UMD-compatible files for distribution. You could avoid some of these manual compatibility woes.

Reply to this email directly or view it on GitHub:
#259 (comment)

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.

Publish to npm for browserify support
3 participants