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

Don't call noConflict by default when loaded via AMD. #165

Merged
merged 1 commit into from
Dec 12, 2013

Conversation

russelldavis
Copy link

Calling noConflict() makes it a pain to load the plugins, which depend on the
global variable (which is why jQuery's AMD handler, for example, doesn't do
that). For those that want noConflict(), that can be done with a wrapper (see
http://requirejs.org/docs/jquery.html#noconflictmap)

Calling noConflict() makes it a pain to load the plugins, which depend on the
global variable (which is why jQuery's AMD handler, for example, doesn't do
that). For those that want noConflict(), that can be done with a wrapper (see
http://requirejs.org/docs/jquery.html#noconflictmap)
@mattrobenolt
Copy link
Contributor

So this is something that I'm not too familiar with personally, but this directly conflicts with a previous pull request to add the noConflict. #109

I originally had it this way.

@mattrobenolt
Copy link
Contributor

Oh, so the issue is because of the new plugins. That makes sense.

How are you using it now? Since the plugins were honestly designed to just be concatenated together and served from one script tag. Is this all getting shoved into a build system? Or is it coming from within a script tag and going through AMD?

@mattrobenolt
Copy link
Contributor

What I'm getting at is that I think if I just change the build system and include plugins within the main closure, this wouldn't be an issue and maintain existing behavior. Would that solve the problem?

@russelldavis
Copy link
Author

I'm loading the plugins via requirejs (using shim config). Your proposal would sort of solve the problem, but it seems like it defeats the purpose of them being plugins (i.e., making them optional).

Another option would be to make the plugins support AMD.

But, imagine a third party developer releases a raven-js plugin. It won't be included in your build, and it might not support AMD. That brings us back to the original problem, which is why I think removing noConflict() is the right solution.

@mattrobenolt
Copy link
Contributor

Yeah, I think that's fair. Due to the new plugin architecture, I think that's a fair trade-off. Can you add to the documentation about the noconflict thing with require.js? That way I can at least point users to that who have the issue.

I'll put some thought into making plugins AMD aware. There's also this browserify stuff that may make sense, not too sure yet, haven't played with it.

Thanks for the feedback. :)

@russelldavis
Copy link
Author

Thanks. Yeah, browserify is nice. It avoids annoying issues like this (see the 'Disappearing globals' section in this post which talks about another library that had this exact same problem).

Anyway, taking a quick look at the docs, I don't see an obvious place to mention this. Considering my change is what I would consider standard (cf. jquery), perhaps it's enough to just point anyone questioning it to this PR?

@juriejan
Copy link
Contributor

Thanks! I also need this. You might want to re-build the distribution file. It doesn't contain the change yet.

mattrobenolt added a commit that referenced this pull request Dec 12, 2013
Don't call noConflict by default when loaded via AMD.
@mattrobenolt mattrobenolt merged commit d81942a into getsentry:master Dec 12, 2013
@mattrobenolt
Copy link
Contributor

This is in the 1.1.3 release. Thanks. :)

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.

4 participants