Opened 4 years ago

Closed 4 years ago

Last modified 2 years ago

#13449 closed bug (notabug)

jQuery UI assumes that jQuery is loadable via AMD just because an AMD environment is present.

Reported by: maratbn Owned by:
Priority: minor Milestone: none
Component: ui.core Version: 1.11.4
Keywords: Cc:
Blocked by: Blocking:

Description

Certain websites, especially the kind which may mix various 3rd party plugins / modules / widgets on one page, such as websites running WordPress and similar CMSes, may end up with a mix of various JavaScript loading environments present on a single page. For instance, one CMS plugin may load an AMD environment such as RequireJS via a <script> tag, and another plugin, unaware of the AMD-using plugin, may load jQuery and jQuery UI also via <script> tags.

The current implementation of jQuery UI core assumes that if an AMD environment is detected that means that jQuery is also available via AMD, and immediately attempts to load it via that method. However, as described above, that is an incorrect assumption, and this logic will cause an error if jQuery is not actually available via AMD.

As demonstrated here: http://jsfiddle.net/wp9ebxbL/

Make sure to open browser JavaScript console before clicking 'Run' to see the error from RequireJS.

jQuery UI needs to first check if jQuery is in fact available via AMD before trying to load it, and if not, then it should just revert to using jQuery in the global namespace.

Change History (6)

comment:1 Changed 4 years ago by maratbn

diff --git a/ui/core.js b/ui/core.js
index 9ac196d..57cabd8 100644
--- a/ui/core.js
+++ b/ui/core.js
@@ -15,7 +15,10 @@
 //>>demos: http://jqueryui.com/
 
 ( function( factory ) {
-       if ( typeof define === "function" && define.amd ) {
+       if ( typeof define === "function" && define.amd &&
+                typeof require === "function" &&
+                typeof require.specified === "function" &&
+                require.specified("jquery") ) {
 
                // AMD. Register as an anonymous module.
                define( [ "jquery" ], factory );

A patch for this issue might look like something like this above; however, this exact patch should not be applied because the functions 'require(...)' and 'require.specified(...)' are only available in RequireJS and are not a part of the AMD standard at this time.

comment:2 Changed 4 years ago by Scott González

Resolution: notabug
Status: newclosed

This is definitely not a common implementation. You should just create a shim that exposes jQuery via AMD. Having AMD support that mixes in global support doesn't make too much sense.

comment:3 in reply to:  2 Changed 4 years ago by maratbn

Replying to scott.gonzalez:

This is definitely not a common implementation. You should just create a shim that exposes jQuery via AMD. Having AMD support that mixes in global support doesn't make too much sense.

I think you misunderstood me. I'm not talking about deliberately creating such an implementation.

What I'm saying is that under certain circumstances, such an implementation can be unintentionally created when 2 unrelated bundles of code, one that uses jQuery / jQuery UI, and another that uses AMD may end up deployed together on the same website, with the developers of the original code having no control over how / where their work is deployed.

The example I provided in the description above is that this can happen on any WordPress site. Specifically, the administrator of the WordPress site may install one plugin / theme that loads jQuery / jQuery UI globally, and then he might also install a completely unrelated plugin / theme that loads an AMD system such as RequireJS.

THE ORIGINAL PLUGIN / THEME DEVELOPERS HAVE NO CONTROL OVER THIS DEPLOYMENT, BUT SUCH A PLUGIN / THEME COMBINATION WILL BREAK THE SITE.

The current implementation of jQuery UI makes it unsuitable for use in any plugin intended for plug-and-play 3rd party deployment, because under the conditions described above, the current jQuery UI logic in such a plugin will break the site.

I disagree with your statement that this is not a common implementation. WordPress is a very popular CMS, and it is very common for WordPress administrators to mix multiple unrelated 3rd-party plugins / widgets on a single site.

I think that this problem should be considered a serious bug that needs to be fixed if jQuery UI is to be intended to be used safely in deployments outside the control of the original developers that integrated it into their work.

comment:4 Changed 4 years ago by maratbn

You should just create a shim that exposes jQuery via AMD.

Earlier I forgot to explain that 'shim' is a RequireJS/AMD term, and this bug is not strictly about using jQuery from inside AMD, which is what a shim might be used for.

In the scope of this bug, we don't even want to use AMD, we just don't want to have a conflict with some other 3rd party module on the page that happens to be using AMD.

And even if we were using AMD, then conversely, we would not want our AMD-using module to break some other module on the page that happens to be loading jQuery and jQueryUI directly via <script> tags, regardless of whether we used RequireJS shims or not.

This ticket has been closed prematurely, as a result of a fundamental lack of analysis and understanding as to what it is really about.

I predict that more people will experience difficulties due to this incompatibility, so I'm still glad that I documented it here, as at least it will give them an explanation as to what is going on.

Last edited 4 years ago by maratbn (previous) (diff)

comment:5 Changed 4 years ago by maratbn

I've been hearing from people stumbling onto this problem too, asking me how I handled it.

My solution was to tweak the RequireJS and all AMD-compatible modules I'm using in such a way that I "namespaced" the global function names 'define(...)', 'require(...)', and 'requirejs(...)' to be something like '_my_namespace__define(...)', '_my_namespace__require(...)' and '_my_namespace__requirejs(...)'. This way my plugin and any 3rd party plugin end up on separate namespaces, so there can be no collision.

Here's a diff of how I tweaked RequireJS:

--- require_js-2.1.20-src.js    2016-02-16 22:28:53.221576000 -0800
+++ require_js-2.1.20-src--tweaked--2015-09-24--01--namespaced--plugin_StripePaymentPress--45576dbca1a4f9ff7385a89c3c1f6db4917fe2c1.js  2016-02-16 22:28:53.225578000 -0800
@@ -8,6 +8,8 @@
 /*jslint regexp: true, nomen: true, sloppy: true */
 /*global window, navigator, document, importScripts, setTimeout, opera */
 
+(function() {
+
 var requirejs, require, define;
 (function (global) {
     var req, s, head, baseElement, dataMain, src,
@@ -2101,3 +2103,9 @@
     //Set up with config info.
     req(cfg);
 }(this));
+
+window._plugin_StripePaymentPress__requirejs = requirejs;
+window._plugin_StripePaymentPress__require = require;
+window._plugin_StripePaymentPress__define = define;
+
+}).call(this);

And here's how I tweaked jQuery:

--- jquery-1.11.3.js    2016-02-16 22:28:53.201565999 -0800
+++ jquery-1.11.3--tweaked--2015-09-24--01--require_js_namespaced--4c456884bd01dd192d549715b5ef4312f4f82b12.js  2016-02-16 22:28:53.193561999 -0800
@@ -1,3 +1,8 @@
+
+
+(function(define, require, requirejs) {
+
+
 /*!
  * jQuery JavaScript Library v1.11.3
  * http://jquery.com/
@@ -10349,3 +10354,8 @@
 return jQuery;
 
 }));
+
+
+})(_plugin_StripePaymentPress__define,
+   _plugin_StripePaymentPress__require,
+   _plugin_StripePaymentPress__requirejs);

(If you add the 'window' prefix to the jQuery tweak as well it might not work on node.js if you want to share the same codebase between environments.)

Original RequireJS: https://raw.githubusercontent.com/maratbn/RainbowPayPress/2bd1f5f634074312ecd21939e29f747d947388c7/plugin/js/lib/require_js-2.1.20-src.js

Tweaked RequireJS: https://raw.githubusercontent.com/maratbn/RainbowPayPress/2bd1f5f634074312ecd21939e29f747d947388c7/plugin/js/lib/require_js-2.1.20-src--tweaked--2015-09-24--01--namespaced--plugin_StripePaymentPress--45576dbca1a4f9ff7385a89c3c1f6db4917fe2c1.js

Original jQuery: https://raw.githubusercontent.com/maratbn/RainbowPayPress/2bd1f5f634074312ecd21939e29f747d947388c7/plugin/js/le_requirejs/lib/jquery-1.11.3.js

Tweaked jQuery: https://raw.githubusercontent.com/maratbn/RainbowPayPress/2bd1f5f634074312ecd21939e29f747d947388c7/plugin/js/le_requirejs/lib/jquery-1.11.3--tweaked--2015-09-24--01--require_js_namespaced--4c456884bd01dd192d549715b5ef4312f4f82b12.js

So you just take any AMD module you want to use, like jQueryUI, and tweak it just like jQuery above. Just encase the whole module into an anonymous function, and then pass that function the global variables corresponding to the new names (that you chose in your RequireJS tweak) of the default AMD functions. The cool thing about this is that not only does it not require messing with the module internal logic, but this approach is completely transparent to the 'r.js' optimizer.

Check out my build.js here: https://raw.githubusercontent.com/maratbn/RainbowPayPress/2bd1f5f634074312ecd21939e29f747d947388c7/build.js

My build script: https://raw.githubusercontent.com/maratbn/RainbowPayPress/2bd1f5f634074312ecd21939e29f747d947388c7/do_minify_le_requirejs

comment:6 Changed 2 years ago by Steve Simitzis

Hello! Please consider fixing this. This exact problem is breaking my code, and should most definitely be considered a bug.

Note: See TracTickets for help on using tickets.