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.

Sign up for GitHub

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 support for commonjs modules #206

Closed
wants to merge 3 commits into from
Closed

added support for commonjs modules #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ed8ccc
added support for commonjs modules
zeevl Apr 28, 2014
be34417
Revert "added support for commonjs modules"
zeevl Apr 28, 2014
d8b7bab
removed dist changes
zeevl Apr 28, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/raven.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// If there is no JSON, we no-op the core features of Raven
// since JSON is required to encode the payload
var _Raven = window.Raven,
hasJSON = !!(window.JSON && window.JSON.stringify),
hasJSON = !!(JSON && JSON.stringify),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with window.JSON here? window is referring to this. Which you can see in the _footer.js ending closure. Wouldn't that context be correct in your case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In browserify (and commonjs in general i think?) this refers to the module, not the window.. so in this case window is just an empty object. removing the window qualifier seemed like a much less invasive change than renaming the initial function parameter..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi, here's the browserify require code:

require = (function e(t, n, r) {
    function s(o, u) {
        if (!n[o]) {
            if (!t[o]) {
                var a = typeof require == "function" && require;
                if (!u && a) return a(o, !0);
                if (i) return i(o, !0);
                throw new Error("Cannot find module '" + o + "'")
            }
            var f = n[o] = {
                exports: {}
            };
            t[o][0].call(f.exports, function(e) {
                var n = t[o][1][e];
                return s(n ? n : e)
            }, f, f.exports, e, t, n, r)
        }
        return n[o].exports
    }
    var i = typeof require == "function" && require;
    for (var o = 0; o < r.length; o++) s(r[o]);
    return s
})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 can confirm that this is necessary to get this working.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't make this change. This is very significantly different in behavior.

(JSON && JSON.stringify) will now raise a ReferenceError instead of returning undefined as window.JSON would.

lastCapturedException,
lastEventId,
globalServer,
Expand Down
8 changes: 6 additions & 2 deletions template/_footer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
// Expose Raven to the world
window.Raven = Raven;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being done on purpose. We need to globally expose Raven unless explicitly calling Raven.noConflict().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was moved to the else block of the amd checks, i.e. only expose it globally if its not otherwise defined or required as a module.

I also needed to move it as I'm using this in a firefox add-on, which will barf if you try to modify the window object... so Raven.noConflict() doesn't work.

(Yes.. firefox has their own commonjs implementation which does end up keepin the window object as this.)

// AMD
if (typeof define === 'function' && define.amd) {
define('raven', [], function() { return Raven; });
}

else if (typeof module == 'object') {
module.exports = Raven;
}
else {
window.Raven = Raven;
}
})(this);