Remove conditional loading

[yuchi]

IMHO conditional loading poses more issues on debugging than features and help.

The issues I had this far are all related to what I call the surprise effect. While you’re debugging something that triggers a conditional loaded module you don’t have any idea that this is the case. That’s because the triggers are defined in the other module, not in the parent/main one.

So what I expect is (pseudo-code)

// navigation-interaction.js

import { isTouch } from 'touch-utils';
import { NavigationInteractionBase } from 'navigation-interaction-base';
import { NavigationInteractionTouch } from 'navigation-interaction-touch';

let NavigationInteraction = isTouch() ?
  NavigationInteractionTouch :
  NavigationInteractionBase;

export NavigationInteraction;

If I read the content of navigation-interaction I cannot not see the reference and therefore I know of the presence of a different module to read and study about.

And IMHO this is not a good idea anyway. All the code should happen in the same place, with conditionals in place.

import { CLICK_EVENT } from 'touch-utils'; // 'touchend' or 'click'
A.one('#navigation').on(CLICK_EVENT, onClick);

Take this as food for thoughts. I simply don’t like the fact that (in Liferay) things can behave so differently without my ‘consent’ or knowledge.

[ipeychev]

When we discussed what we should keep from YUI Loader and what we should drop, we were torn about the conditional loading. Some said we have to remove it, others - we should keep it.

The argument for keeping it was that otherwise you may end up with much more loaded JS than what is actually needed.
Also, in the example above another request(s) will be made if Loader with lack of support for conditional loading is being used. And if you don't have SPDY or HTTP/2 this might be expensive.

You are right however that changing the behavior of a module in this way may be confusing. However, to allow loading much more JS than needed didn't look as a good option neither.

@natecavanaugh @eduardolundgren

[juandopazo]

A lot of discussion around conditional loading in the context of ES6 modules and the browser Loader: systemjs/systemjs#9.

[yuchi]

Great read and discussion, thanks @juandopazo!
For sure the most confusing thing is the placement of the trigger.

Even if I know I’m late to the party and the following has already been proposed, what about placing the triggering and the condition on the parent and not triggered one.

[juandopazo]

Yes, that was the most complicated part of the discussion. My conclusion was similar to yours:

1. Write your modules so that they work without a Loader. That means follow the first snippet in your original post. That module doesn't require anything from the Loader. It downloads both the "base" and "touch" versions but it works. This makes sure that the code we distribute works on its own and doesn't have an implicit dependency on Systemjs/the-YUI-Loader/some-other-loader.
2. Optimize using configuration. Somewhere at the top of your app add a configuration for the Loader that enables conditional loading. In production it'll only download the version it needs. During development you can disable conditional loading and figure out what's going on. It's complicated to document and distribute configuration, but it can be worth the performance benefit.

Pretty much all options have their drawbacks though.

[yuchi]

Another point to the ‘require all anyway’ is that when I'm debugging some behavior (personally I prefer to read the code while executing it) there's no trace that there's another ‘hidden chapter’ to be read.

In browserify there's the ignorability of modules, when you require an ignored module you get an empty exports instead. Something along those lines could do the job.

[juandopazo]

That's not a bad option. We could turn conditional loading into "conditional skipping". I'd have to think about it a bit.

[ipeychev]

@juandopazo I used to follow this thread but after a few months I stopped. From what I read now, it is still undecided in SystemJS and the proposal is to have something like DSL in the string in "from" clause, isn't it? This one I'm not sure if I like much.
I more agree with your proposal.

I'm trying to imagine how will that work on practice. If we get the classic example with drawing, this will mean we will have code like this:

import {drawingBase} from 'drawing/drawing-base.js';
import {SVG} from 'drawing/svg.js';
import {VML} from 'drawing/vml.js';
import {Canvas} from 'drawing/canvas.js;

if (browser-supports-svg) {
    // create an instance of drawing SVG
} else if (browser supports VML) {
   // create an instance of drawing VML
} else if (browser supports Canvas) {
   // create an instance of drawing Canvas
}

Now in production you want to avoid loading of some of these files. You will go and add configuration to each of the modules to load only if some condition is met. It will work, but you will have to keep in sync the code and module conditions in the config file. Now you have two problems instead one.

[juandopazo]

Yes, but using something like a magic string in the from clause adds an implicit dependency on Systemjs or some other specific Loader. So it's a matter of deciding between a hard dependency or a configuration hurdle.

[yuchi]

IMHO if conditional loading/skipping (I like the name) is not explicitly permitted by ES than it’s for sure something that happens on the loader. If it’s something that requires the loader to have a specific feature then there’s only one sane option: graceful degrade.
So whatever the solution is (given that ES is not going to include it) the resulting code must work/behave exactly the same with or without conditional loading.

The conditional features must preemptively optimize obviously useless code.

[ipeychev]

@juandopazo About adding DSL in from clause, I totally agree. This idea sucks.
Looking again on the modules syntax in ES6, maybe other options would be to:

• add something like hooks
• extend the syntax with something like when clause (since we cannot have import in if):

import {juan} from 'juan.js' when [statement which returns truthy value here];
// In this way the developer can write:
import {svg} from 'drawing/svg.js' when SVG_SUPPORTED;

// or
import {juan} from 'juan.js' when function() { return true-if-browser-supports-svg};

This is shot in the dark, of course, I guess both hooks and extending the syntax were already discussed by TC39 and discarded for some reason. If you know what was it, please share.

If there is no way to extend the syntax or add hooks, then the possibility with less drawbacks would be to return to the idea to add custom configuration for now, paying the price that the developer should look at it in order to understand what exactly is going on.

[juandopazo]

The problem with when is that it would break the static nature of modules. Now in some cases your imports/exports would contain one thing and in other cases it won't. That's dynamic by nature.

[yuchi]

The idea itself of conditional loading is completely against any kind of static analysis.

One of the next ES versions’ goal is reach the tooling abilities from other mainstream programming languages. Currently to know what a module exports we have to resort to heuristics.

[juandopazo]

The idea itself of conditional loading is completely against any kind of static analysis.

Not so much. In this scenario static analysis would tell you if you failed to implement something in one of the modules:

import { line as line1, circle as circle1 } from 'svg';
import { line as line2, circle as circle2 } from 'vml';

var line   = supportsSVG ? line1 : line2;
var circle = supportsSVG ? circle1 : circle2;

export { line, circle };

[yuchi]

Yeah, sorry. my comment was too much extreme. What i meant is more along the lines of:

[...] conditional imports are against [...]

[ipeychev]

The problem with when is that it would break the static nature of modules. Now in some cases your imports/exports would contain one thing and in other cases it won't. That's dynamic by nature.

Umm, however no one forces you to add when clause or hooks to your modules. If you consider the static analysis as more important than performance - go ahead and import everything. If you consider the performance as more important - add hooks/when clauses on the price of incomplete static analysis. Why not giving people choice?

[yuchi]

IMHO this is not something that can work on lang-level. We’re not talking about the browser anymore.

I’m thinking about Titanium SDK, where the dynamic nature of imports just makes impossible to create a stable and optimized bundle using CommonJS packages.

Having a standard, static, import/export feature on language side means that I can scan the whole dependency tree and I know that everything will work as expected.
You want to conditionally skip or trigger modules? then use a loader-specific feature.

This is my way of looking at things. I still completely understand the rationale behind it. In Titanium SDK this could be used for platform-specific code.

[eduardolundgren]

I am personally not a big fan of relying on the loader to handle conditionals. The loader should do what it is supposed to do, which is to load the module you are requesting for. Nothing more, nothing less.

Often, testing conditionals for every module calculation can be time consuming, transferring the extra bytes may be a better option than the calculation.

[yuchi]

The Liferay codebase is a good candidate for some quantitative analysis of costs vs benefits.

[ipeychev]

The fact that you can do something does not mean you have to do it always. It means that if you really need it - you can use it. In Git usually it is strongly discouraged to use git push -f, but in some situations it is useful, you are good to go and the option is still there.

It is hard for me to imagine that a condition may be slower than the network traffic but even in this case you are barking up the wrong tree. Those who should be blamed is the developer who implemented it, not the fact that such feature exists in the Loader.

It is clear that loading modules conditionally should be handled by the Loader. The example which extends the from clause was shown as one of the a few options we have in order to resolve the original complaining from @yuchi :

While you’re debugging something that triggers a conditional loaded module you don’t have any idea that this is the case. That’s because the triggers are defined in the other module, not in the parent/main one.

@yuchi 's suggestion how to resolve this was to completely remove the conditional loading even from the Loader. It is not allowed in the language, it won't be allowed in the Loader, so we completely forget about it and we happily ship all JS, including those which is only suitable for IE8, but the user is using Spartan browser. This does not sound as a dream became reality.

[yuchi]

In Git usually it is strongly discouraged to use git push -f [...]

In Git you have full ownership of what you do. When you do something ‘wrong’ you’re hurting yourself and your collaborators. But the implications of (for example) forcing a push to an unsynced remote are clear and perfectly known to the wrong-doer.

Conditional loading/skipping has influence on what depends on you, and generally on a broader and subtler level.

Still the idea of moving the metadata from the triggered to the trigger is still incredibly valid IMHO.

This does not sound as a dream became reality.

Sadly true.

[yuchi]

BTW, as a possible skip syntax that preserves high level behaviour:

// my-util-touch.js

import skip from 'if-not-touch';

// statically resolvable `skip` property
export const meta = { skip }

export function initialize() {
  // This will be replaced with a NOOP when conditional skipping is supported
  if (test()) return;
}

// my-util.js

import { initialize as initTouch } from 'my-util-touch';

initTouch();

[yuchi]

Even better:

// my-util-touch.js
export { test as skip } from 'if-not-touch';
export default function () {
  // This will be replaced with a NOOP when conditional skipping is supported
};

[eduardolundgren]

Indeed for @yuchi's argument about conditionals hiding complexity.

On the other hand, it is relevant to not completely ignore JavaScript execution argument on this conversation, specially when you imagine a limited mobile phone, calculating the topological search for all modules + checking arbitrary conditionals for each step doesn't smell like the best approach.

Reserve time for browser layout and rendering (200 ms):
The process of parsing the HTML, CSS, and executing JavaScript takes time and client resources! Depending on the speed of the mobile device, and the complexity of the page, this process can take hundreds of milliseconds.

Optimize JavaScript execution and rendering time:
Complicated scripts and inefficient code can take tens and hundreds of milliseconds to execute.

Blocking above-the-fold JavaScript can delay loading other resources on the page.

[ipeychev]

Sure, there should be some balance. Here I would add that adding unnecessary JS (or CSS, or HTML) may slow down the application even more, because you will likely miss the 14KB limit on the first RTT.

How costly of calculating the conditional modules right now in the Loader? Here is the is the whole code, this is being executed during the process of building dependencies:

_processConditionalModules: function (module) {
        var conditionalModules = this._configParser.getConditionalModules()[module.name];

        // If the current module has conditional modules as dependencies,
        // add them to the list (queue) of modules, which have to be resolved.
        if (conditionalModules && !module.conditionalMark) {
            var modules = this._configParser.getModules();

            for (var i = 0; i < conditionalModules.length; i++) {
                var conditionalModule = modules[conditionalModules[i]];

                if (this._queue.indexOf(conditionalModule.name) === -1 && this._testConditionalModule(conditionalModule.condition.test)) {

                    this._queue.push(conditionalModule.name);
                }
            }

            module.conditionalMark = true;
        }
    }

As you see, how slow will be the processing of conditional modules, depends on the developer. If the currently processed module does not have any conditional modules - NOOP. If he adds some conditions - he is in charge of making them as light as possible. If you add 15 conditional modules to another one and they have heavy calculations - then there is something totally wrong in your program and you should fix it.

The part for conditional loading is what worries me less from performance point of view or how will we support this. The code is just a few lines, if there are no conditional modules there will be NOOP, so... Removing it will be easy. If needed, it will be trivial for people to add it back via plugin or whatever. Let's see what will happen with this issue in SystemJS first, since they are dictating the rules.

[natecavanaugh]

Since I was the one heavily voting in favor of conditional loading, I would like to add some context to the reasoning.
The main reason isn't only because of browser feature sets, but because the conditions could literally be anything.

Netflix has this issue as well, and if you have some time, this is a very interesting read: http://queue.acm.org/detail.cfm?id=2677720

For them, it's that they want to run A/B tests, and select an entirely different implementation of a component depending on any number of conditions.

For us, right now it's based on features of the DOM and browser environment, but I could imagine in the future it could also possibly be other conditions (permissions, location, or some other context).
So it's not so much just performance, though that does play into it, but also providing a contextually appropriate implementation.

To me it seems like having the feature testing done on the code level will just lead to some unmaintainable code.

My original proposal, which the other guys weren't fond of, was to just store that metadata in a comment, or Iliyan thought, as a function expression that we could parse. This would allow us to define the conditions, but be able to keep them innocuously stored with the code.

As @yuchi mentioned, this does have some surprise effect, but I think that's part of the nature of the interceptor pattern.

What level this should all get stored on, it's tough to say. I would imagine it would be nice if there were a conditional plugin for the loader, so you could opt into it (so you don't have to worry about it at all if you just want a simple loader).
But it still leads to the question of where to store this metadata, and that, I'm not 100% decided on what the best option is.

[juandopazo]

@natecavanaugh I was thinking about conditions in comments yesterday! We could use comments to generate configuration for the loader and even as metadata for documentation.

[yuchi]

I’m sorry to have brought to the surface a feature that has been considered stable in the context of lfr-amd-loader. But because whatever will be shipped with Liferay is going to be on my shoulders for years… well, you get the idea! 😅

@natecavanaugh brought up an interesting topic, that is conditional loading for reasons outside feature coverage of the environment.

Yet there are two different different challenges at the same time IMHO. Liferay (for example) it’s not simply a product anymore (it has never been one actually.) So you have to provide at the same time a great development platform and a great product.

To build a great platform you have to predictable, and probably waaay later clever.

If conditional loading is a good idea on the framework, loader level, that doesn’t directly means that it’s a good idea to build a platform out of it. If in Liferay you used conditional triggering more than what you already did (in 4 places) then I probably had to give way more consulting than what I already did. It’s a good thing, it’s business!

Every time I depend on a module, I actually depend on the intersection of its features and predictability.
And you cannot, I repeat, you cannot foresee what devs will depend on; therefore you must think that everything you ship with the platform will be used as a starting point for other modules.

How awkward feels when you monkey patch an object/api? In fact most of the popular (bad metric I know) modules monkey patching other popular modules are actually built by the maintainers of the patched one. Express and similars come to my mind.

That’s because if you monkey patch something you are breaking the contract between the consumer and the provider.
Conditional skipping is just an optimization to cut bytes.
Actual conditional (A/B, permission based …) is ok IMHO in only two different cases:

• for consumers (there’s no contract);
• explicitly stated and easily debuggable.

[eduardolundgren]

Permissions, location etc, are very high level features to be handled on loader modules definition. It can be done programmatically by the developer at other levels of the application.

Conditional loading into modules mapping seems that we are trying to solve everything in the same layer. The OSI model (networking) might not be the best analogy, although, It's divided into seven specialized layers for a reason: Physical, Data link, Network, Transport and so forth. The network layer is responsible for packet forwarding, routing etc. The Application for protocols etc. Separation of concerns. We don't need to solve everything in the same place.

[ipeychev]

juandopazo: I was thinking about conditions in comments yesterday! We could use comments to generate configuration for the loader and even as metadata for documentation.

@juandopazo, we already do this with the difference that the condition is not stored in comments, but:

• in a special META function in the ES6 source or
• in the transpiled (or written manually) AMD file as fourth parameter of define function

There is a special program in this repository, called config-generator which parses the transpiled to AMD files and generates the final config.js file automatically, taking care of the conditions too. Read here for more info.

Examples for storing the conditional tests in both ES6 sources or in the define function:

// conditional-module1.js
import {log as logBase} from 'aui-base';

// more code here...

(function META() {
    return {
        condition: {
            test: function() {
                return true;
            },
            trigger: 'nate'
        },
        path: 'nate.js'
    };
});

and in the define:

define('aui-event', ['exports'], function (exports) {
  // more code here ...
  exports.log = log;
}, {
    condition: {
        trigger: 'aui-test',
        test: function() {
            var el = document.createElement('input');

            return ('placeholder' in el);
        }
    },
    path: 'aui-dialog.js'
});

[juandopazo]

@ipeychev that's pretty cool! 😎

[eduardolundgren]

It doesn't look bad, indeed @ipeychev. Very clean approach, you almost convinced me to like conditionals :)

[ipeychev]

Haha :)

[natecavanaugh]

@yuchi
I think we may be on the same page, but I may be miscommunicating what I'm thinking of when I say conditional loading.
I definitely think every component should have a public contract with it's API, that as long as the implementation actually implements that contract, there shouldn't be any problem. That contract should be documented, and when it needs to change, as much as humanly possible, it should be deprecated and migrated.

When I think of conditional loading, I think of something like the YUI Uploader where the implementation can change to suit the needs of the environment, and the API is fairly consistent across implementations, while exceptions are documented.

I don't think of conditional loading as monkey patching necessarily, but more as providing different implementations for the same contract. Sure, end devs could poke into the code and modify private methods, but the only other option for that is to make the private methods actually private via closures, and completely prevent any extension. That approach has never been popular with third parties in the past, though.

Also, what I like about the YUI method of conditional loading is that each implementation can be used in isolation if you don't want the conditionally loaded one.
For instance, you can load uploader and Y.Uploader will point to either Y.UploaderHTML5 or Y.UploaderFlash depending on the condition.
However, end developers can just completely forego any conditional loading "magic" and just use the one they want.
This to me, seems the best of both worlds, but that's just my opinion, and I look forward to hearing yours :)

(Though one small nitpick, I would disagree that Liferay isn't a product. The hardest part about Liferay, IMHO, is that it's both a product and a framework, very much like an operating system. For some clients, we are the product, and they don't really need much customization beyond a theme; for many, that's just a starting point and they need a lot more. But like Apple and Microsoft, we have to have a functional product that solves real needs on it's own, but also a healthy platform for people to build upon... I think it's critical for us to have both. But again, that's a minor thing :D).

@juandopazo about the comments, I can't take 100% credit for the idea. Wordpress has been doing it for their themes and plugins since they started back in 2004, I think, but I actually like @ipeychev's approach way better (and it's far less fragile than parsing the comments), so great job Iliyan :D