Handle optional dependencies in loadLanguages()

[Golmote]

Fixes #1393

[mAAdhaTTah]

I'll probably be able to review this PR this weekend, just FYI.

[Golmote]

Sure, take your time!

[mAAdhaTTah]

I think this is a really nice solution to this problem, and I think we can do it without exposing withoutDependencies (documented or otherwise). Let me know what you think!

[mAAdhaTTah]

We do this a couple times; we could definitely do this once and remove meta and reuse that array. If we're mostly going to be doing iteration, we could just map over the keys and turn them into values, so end up w/ an array of languages. If we want to go a step further, we could tag the objects w/ a loading property and remove the withoutDependencies information and just recurse until you hit a component that's already being loaded.

This would be a bit more stateful but shouldn't be hard to implement and would allow us to hide the implementation details from consumers. I don't really love exposing withoutDependencies, if if it's not technically public API, and that would do away with it nicely.

[Golmote]

I'm not sure I understand your whole idea here.

We could indeed process the languages only once, though. We could build a structure that maps a language to its "peerDependencies" and therefore just access it by key. Something like:

var dependentsMap = null;
function getOptionallyDependents(mainLanguage) {
    if (!dependentsMap) {
        dependentsMap = {};
        /* Build a structure like:
        dependentsMap = {
            "fortran": ["pure"],
            "coffeescript": ["haml", "pug"],
            ...
        }
        */
    }
    return dependentsMap[mainLanguage] || [];
}

Is it what you meant?

[mAAdhaTTah]

If we change this to:

if (!components.languages[language]) {
  console.warn('Language does not exist ' + language);
  return;
}

...then we can save an indentation. I like early returns :)

[mAAdhaTTah]

peerDependencies maybe?

[mAAdhaTTah]

One-liner these two?

[mAAdhaTTah]

Is the purpose of withoutDependents to avoid an infinite loop? Are we sure it recurses through everything? If it is, maybe we don't need it (see above).

[Golmote]

Actually no, it wasn't to prevent an infinite loop. I think an infinite loop can still happen if we have circular dependencies, hopefully this is not the case at this time AFAIK.

It was supposed to be an optimisation, because you need to reload a component so that it takes its updated "peerDependencies" into account, but you don't need to reload the whole "inheritance" tree. There's already a lot of files being required there, so I tried to keep the amount as low as possible.

[Golmote]

I updated the PR to address your comments.

Regarding withoutDependencies, I kept it for now, I still need to understand what you meant in your first comment. But I removed it from the export so it's not exposed anymore.
And the map of dependents is now built only once.

Let me know what you think.

[mAAdhaTTah]

But I removed it from the export so it's not exposed anymore.

This may be fine; the idea is essentially to keep track of what languages we're in the process of loading and avoid reloading them if we hit them again via circular references. Doing it that way would ensure we recurse down all the way down instead of (what looks like?) one level down.

But maybe that's not actually an issue? This is certainly a simpler implementation.

[mAAdhaTTah]

Also, I think this PR may be needed by the babel plugin to ensure we load everything in the correct order.

[Golmote]

@mAAdhaTTah Since we don't seem to have any circular dependencies at the moment, I would suggest we keep it this way, at least for now.
Did I address all your requested changes? It this good to be merged?

[mAAdhaTTah]

Yeah, this is good. Just need to fix the conflict in components.js.