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

fix for jquery plugins being lost, if bootbox is required later in the execution #436

Merged
merged 1 commit into from Sep 25, 2016

Conversation

abhisekpaul
Copy link
Contributor

if required couple of jquery plugins before requiring bootbox, when you require bootbox, the plugins used to get lost in the "$". Moreover there were 2 instances of $ hanging around in bootbox, one from global scope and the other from closure.

The fix is to only require jquery if not already defined.

in short:
require('jquery');
require('bootstrap');
require('some-plugin');
...
much later,
require('bootbox');

expected:
$.fn.modal should be defined

actual:
$.fn.modal is undefined

Copy link
Collaborator

@makeusabrew makeusabrew left a comment

Choose a reason for hiding this comment

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

Happy with this if you can just follow the style guide everywhere else in the library - condense the if / else braces and use two space indentation.

@makeusabrew makeusabrew merged commit 66ce0af into bootboxjs:master Sep 25, 2016
@makeusabrew
Copy link
Collaborator

Nevermind, I'll sort the style issues. Thanks.

@lddubeau
Copy link

This change should be reverted.

If there's a problem of the kind the OP reported, the solution is to configure the module loader or the module bundler to avoid loading two conflicting instances of jQuery. In other words, the solution is not to change bootbox but to control the environment in which it runs so that it gets a sensible value when bootbox code calls require("jquery"). I've used multiple loaders and bundlers and have never encountered a case where it is impossible to configure these tools to prevent the problem the OP reported. If the OP is using a tool that cannot handle it, I'd argue that this hypothetical tool is defective. The issue reported here is not novel by any means.

Conversely, I am 100% certain that there are usage scenarios that currently work fine with bootbox that the new code will break. For instance, there exist projects that need to bundle jQuery with the project's code but do so in a way that is invisible to any jQuery that may have been previously loaded outside the project's bundle. The change here will make it so that the jQuery included in the bundle will be ignored in favor of any previously installed jQuery, which is definitely a surprise, and a breaking change.

else
{
module.exports = factory($);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, and you're using different if (...) { ... } else { ... } structure than this project...

sumityadav pushed a commit to sumityadav/bootbox that referenced this pull request Feb 1, 2018
fix for jquery plugins being lost, if bootbox is required later in the execution
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.

None yet

4 participants