Allow namespacing the loader

[jbalsas]

Currently, the loader always exposes a global require and define methods. This causes some not-so-unexpected interoperability problems with other libraries that either present an anonymous umd wrapper or contain their own loader implementation (see Dojo).

To solve this issues, it would be nice to be able to configure the namespace where the loader methods are exposed.

var Company = {};

__CONFIG__.namespace = companyNS;

console.log(window.require, window.define); // undefined undefined
console.log(Company.require, Company.define); // function function

With this option, the loader can be exposed globally by default and hidden behind a namespace if a specific project requires it.

[jbalsas]

What do you think, @ipeychev?

[yuchi]

We could go with a .noConflict() method.

[ipeychev]

@yuchi You mean .noConflict() like in jQuery, don't you? According to their documentation:

$ is just an alias for jQuery, so all functionality is available without using $. If you need to use another JavaScript library alongside jQuery, return control of $ back to the other library with a call to $.noConflict()

.noConflict() then would not assign $ to anything but the whole functionality would be still available via jQuery. In our case, to what would we assign require and define? To nothing?

@jbalsas Was that "namespace" reassignment proposed by a real client? I'm trying to figure out what would be the benefits for a company, let's say Google, to have google.require and google.define in their page.

[jbalsas]

Hey @ipeychev, I was just trying to expose it in a general way. The real client in this case is Liferay Portal. As hard as we (specially you) tried (and suceeded) to create a fully compliant amd loader, the problem is that a lot of the existing web is not 😢 . We are having a lot of problems with clients upgrading and seeing their old code behave in unexpected ways in the presence of our loader.

For this reason, our general plan could be summarized as:

• Add an option to the loader to configure where it gets exported
• We will always expose and use the loader via the Liferay namespace
• We'll add a property or configuration com.javascript.loader.global that when true (default value), will expose the loader methods on the global scope as they're supposed to.
• If someone has conflicts or wants to use a different loader, they can simply turn off the setting and our loader will be hidden behind the namespace and they'll be free to use whatever they want.

I want to keep the default standard behaviour. Expose define and require as global, but have the option to hide it if needed.

Does it make sense?

PS: I don't think the .noConflict() option would work as you explained, that's why I proposed extending the __CONFIG__ object.

[yuchi]

@ipeychev it would re-assign the previous values and return a {require,define} obj, for example. But they could also be accessed through Loader.require/define as SystemJS does with .amdRequire/amdDefine.

[ipeychev]

We currently expose three things: a Loader variable, which is a live instance of the loader, define and require functions.
If we go with .noConflict(), all we should do is to unbind define and require and the functionality will be still available via the Loader variable. However, I dislike jQuery's way of doing configuration with a method and .noConflict() says there will be no conflicts, but doesn't say how.

Maybe what @jbalsas proposes would be the best one - a namespace variable and another one, something like exposeGlobal:

1. If namespace is specified with "Liferay", define and require will become window.Liferay.require and window.Liferay.define.
2. If exposeGlobal property is set, we will will expose require and define globally too and by default it will be true.

Any additional comments, arguments agains this or some other proposals?

Thanks,

[jbalsas]

Hey @ipeychev, this sounds good to me, I was actually going to ask that namespace be an array, so we could pass everything in just one setting, but your proposal sounds even better.

If I understand it correctly:

// Default behaviour
console.log(window.Loader, window.define, window.require); // Loader, function, function

__CONFIG__.namespace = "Liferay";
// exposeGlobal = true should be the default
// __CONFIG__.exposeGlobal = true;

console.log(window.Liferay.Loader, window.Liferay.define, window.Liferay.require); // Loader, function, function
console.log(window.Loader, window.define, window.require); // [undefined|Loader]?, function, function

__CONFIG__.namespace = "Liferay";
__CONFIG__.exposeGlobal = false;

console.log(window.Liferay.Loader, window.Liferay.define, window.Liferay.require); // Loader, function, function
console.log(window.Loader, window.define, window.require); // undefined, undefined, undefined

The only thing unclear to me is wether we should expose as global the Loader life instance as well. Other than that, this looks like a solid approach.

PS: One other thing I don't like about the .noConflict() approach, is that it depends on code running after the Loader has already exposed things... so things may go wrong before we can hide it again...

[ipeychev]

The code you wrote is correct. The argument against .noConflict() is correct too - that is the main drawback of setting configurations using a method.

For backward compatibility, we should expose window.Loader if exportGlobal is true. I don't think it is used now, but on theory someone may set exportGlobal to false and to use window.Loader.define and window.Loader.require to bind require and define to somewhere else. We either keep it, or we drop it and bump the version of the loader to 2.

Thanks,

[jbalsas]

You're right, and we're actually using it to manually configure some additional modules via Loader.addModule... let's keep it and stay on version 1 ☺️

[ipeychev]

Okay then. @yuchi, do you have something else to add before we go with this approach?

[yuchi]

@ipeychev Nothing to add!
@jbalsas IMHO it’s a good idea to force it as Liferay.Loader in Liferay, as we did for jQuery on LR 5.x (it was not available as window.$); otherwise marketplace apps will use Loader.addModule with no predictable result.

[jbalsas]

Hey @yuchi, I totally agree, but as @ipeychev pointed out, that will be already a breaking change. By avoiding it, we can safely leverage this update to the existing Liferay Portal version and give proper notice for a future breaking change. We can definitely hide it by default in the next major.

[ipeychev]

Okay then, all clear. So, some volunteer to write this small piece of code and to see his name written with golden letters in the history of the mankind or I shall do it?

Thanks,

[jbalsas]

Hahaha... I can take care of this and send it your way, unless @yuchi wants to become a contributor 😉

[yuchi]

I’m just a bystanding stalker, don’t worry about me.

[ipeychev]

Hey @jbalsas,

May I ask if you are going to send the PR? Otherwise I will do it.

Thanks,

[jbalsas]

Hey @ipeychev, @brunobasto was going to take over this since I got pulled to other stuff, but he also got re-prioritized 😢

I'll try to get it done right now, and otherwise let you know so you can work on it.

Thanks!

[jbalsas]

Hey @ipeychev, I look briefly into it today, and I'm not sure I'll get it right without messing up the templates.

I'd appreciate if you could work on this and I'll "just" deal with the integration 😢

[ipeychev]

Fixed in v1.5.5

[jbalsas]

Thanks so much!!!!

[mfreeman-xtivia]

So a question about this fix--if I am merely interested in kicking the custom AMD loader out of the global namespace, and don't really need/want to use the custom module environment it provides, how would you recommend that I set the "CONFIG.exposeGlobal" (presumably in a standard portlet?)

[jbalsas]

Hey @mfreeman-xtivia, I actually didn't see this... is this answered by #79 (comment)?

[mfreeman-xtivia]

yes thanks