AMD implementation breaks sites about store.js HOT 11 CLOSED

what a clusterfuck.

Thanks for the detective work.

I've never used amd myself and am not very familiar with it. Are you
confident that changing define(store) to define('store', store) does
not break existing code using store via amd? If yes, I'll take your word
for it.

Feel free to PR.

On Wed, Oct 2, 2013 at 2:14 AM, thomasbachem [email protected]:

I finally found store.js while researching strange Javascript errors that
I've encountered in my logs for some time now.

It seems like some quite popular Chrome plugin uses store.js, so it gets
executed on my site even though I do not use store.js myself.

The problem is that store.js breaks my site! It confuses Dojo's AMD loader
(and probably any other AMD loader too) when being loaded via a script tag
in an AMD environment.

So when a page or browser plugin includes it via <script src="store.min.js"></script> when e.g. the Dojo Toolkit was loaded
before, store.js will call define(store), but the AMD loader will be
unable to find out where that define came from and cannot determine any
module ID. It will then fall back to the last used module ID, which can
break things quite heavily.

So define(store) should be changed to define('store', store) to provide
an explicit module ID.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/78
.

from store.js.

This should not be the responsibility of store.js, rather your AMD loader should shim store.js IMHO.

from store.js.

@thomasbachem, wanna add anything to this? If not, can we close?

Cheers!

from store.js.

Anything more on this? Should we use the suggested define('store', store) fix? Could that break any existing loader? Or should we close this as won't-fix?

from store.js.

Looks fine to me.

Other devs have defined this pattern: https://github.com/umdjs/umd/blob/master/returnExportsGlobal.js

@marcuswestin this should be won't-fix. define('store', store) will just pollute namespace and doesn't actually solve the problem.

from store.js.

Ok, let me go into detail:

The error I essentially get is a "multipleDefine" error from Dojo's AMD loader, as it substitutes the missing module identifier with the last used one, leading to two defines with the same module identifier. This stops the whole Javascript on that page to work.

This error is documented here: http://livedocs.dojotoolkit.org/loader/amd

The most common cause of this problem is loading modules via script elements in the HTML document. Use the loader; don't use script elements.

And a long discussion by the AMD masterminds here: https://groups.google.com/d/msg/amd-implement/c9EcRZyKmvA/QzKvpieLBwQJ

It sounds like the consensus is:
users: "Don't mix script and require(), it's unsupported."
library authors: "Either create a named define (like jQuery), or do NOT use an anonymous define.

from store.js.

I guess you may use define('store', store). Though, IMO, it's odd to be loading js via script tags while at the same time using an AMD loader (requirejs, dojo, etc).

With my comment on 'namespace pollution', this approach typically come with no-conflict function.

This is how jQuery handle AMD in latest 1.x:

// Register as a named AMD module, since jQuery can be concatenated with other
// files that may use define, but not via a proper concatenation script that
// understands anonymous AMD modules. A named AMD is safest and most robust
// way to register. Lowercase jquery is used because AMD module names are
// derived from file names, and jQuery is normally delivered in a lowercase
// file name. Do this after creating the global so that if an AMD module wants
// to call noConflict to hide this version of jQuery, it will work.
if ( typeof define === "function" && define.amd ) {
    define( "jquery", [], function() {
        return jQuery;
    });
}

The dependency param (2nd param) isn't required, depending on what AMD spec you choose to adhere.

from store.js.

From what i understand, its obviously a fault of the plugin, not store.js.
The plugin shouldn't brought in something that pollute your environment, if it keeps doing so, eventually it will break here and there.
Says, what if the plugin brought its own piece of jquery and take over the window.$?

The reasons we shouldn't do define('storejs') is that:

1. The require js user lose the ability to choose the name of the module
2. As long as it is loaded by using <script /> tag, the AMD Implementation will probably still confused about which context it should belongs to. see http://requirejs.org/docs/api.html#multiversion

from store.js.

This may very well have been solved by 7c65e0a. @thomasbachem, can you please verify and/or close?

Thanks for the patience!

from store.js.

I looked at the changes but I don't think they will make much difference. The new version still calls define and still does it anonymously...

I am not even sure the problem is solvable in the first place... From the script author's perspective he can never do it right:

• If he doesn't call define the script doesn't support AMD
• If he calls define without a name (so anonymous), this issue may be caused
• If he calls define with a name, he is going against the recommendations that AMD itself has put forth...

The general recommendation seems to be to either don't use AMD, or use it everywhere. I personally think that's a shame... Maybe they should be more forgiving in the case of unmatched module and just ignore those cases...

Changing to a named module now will break people using it via an AMD loader... So maybe all you can do is check for a flag to suppress the AMD call and convince the plugin guys to set that flag before using store.js...

from store.js.

Sounds like there may not be a good solution at the end of the day :/ Feel free to reopen with more info.

from store.js.