Skip to content

Conversation

zeevl
Copy link

@zeevl zeevl commented Apr 28, 2014

Hey there,

This PR adds support for commonjs modules, browserify etc, sometimes used in environments where the 'window' object isn't available, but JSON is.

Steve

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.

@mattrobenolt
Copy link
Contributor

Definitely remove the changes in dist/*. I compile those when I publish releases. It makes patches really messy.

@clayzermk1
Copy link
Contributor

👍 This patch got browserify working for me.

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.

@mattrobenolt
Copy link
Contributor

Superseded by #261

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