stealjs / steal-conditional

Conditional loading
https://stealjs.com/docs/steal-conditional.html
MIT License
6 stars 1 forks source link

Integrate with the slim loader #43

Closed matthewp closed 7 years ago

matthewp commented 7 years ago

Which the plugin API is figured out for slim, we should make sure that steal-conditional can do whatever it needs to do.

m-mujica commented 7 years ago

Unlike css/less, steal-conditional is not really a plugin. So, we might need to play a little bit with the graph to get it working as a regular plugin

Currently, my main module in the substitution example app transpiles to:

[
  3,
  function(stealRequire) {
    var _lang = stealRequire("locale/#{lang}");
    ("use strict");
    var _lang2 = _interopRequireDefault(_lang);
    function _interopRequireDefault(obj) {
      return obj && obj.__esModule ? obj : { default: obj };
    }
    document.getElementById("header").textContent = _lang2.default.helloWorld;
  }
]

As I see it we have two possible implementation paths here:

Use the current plugin API

For this to work, steal-tools would have to:

Replace the steal-conditional ids, e.g: locate/#{lang} with a slim module id that requires the new conditional slim plugin to be loaded first, then the plugin would take care of:

Things I'm not sure what to do about:

a) if we replace the conditional module id with a numeric id, we are going to loose the condition pattern; we'll have to add a new object to the slim config where we map the faux numeric id to the conditional pattern, something like:

Let's say locate/#{lang} is replaced with 4:

(function(global) {
    global.steal = global.steal || {};
    global.steal.bundles = { "4": 6 }; // this indicates the module id 4 needs to load "6" first
    global.steal.plugins = { "6": 3 };  // 3 is the id of the yet to be created slim conditional version
        global.steal.conditional = { "4": "locate/#{lang}" }; we need to keep the pattern available
})(window);

Also, we need to figure out the numeric id for lang (the condition module), currently there is no config that provides it.

The good:

The bad

Support steal-conditional by extending stealRequire

Looking at the current transpiled version, we could just add an extension to stealRequire so it handles the conditional id. It would to:

The good:

The bad:

Any thoughts @matthewp, still thinking about this.

matthewp commented 7 years ago

I think the latter option is almost required, right? Otherwise it isn't dynamic. In the case where you check navigator.language or something at run-time, it has to be dynamic.

Just don't increase the loader size for non-steal-conditional users :) . I think this path is probably the way to go though.

m-mujica commented 7 years ago

@matthewp regarding the solution we discussed yesterday, I think there is a major design flaw.

if we run steal-conditional before the graph is executed to preload the bundle where the conditionally loaded module code lives, it's possible that during graph execution the state that steal-conditional checks has changed since startup hence the loader might end up with a different module than the one the user "expects".

Unless the loader preloads the bundles of all known variations and stealRequire goes through steal-conditional's logic to delay the evaluation of the condition module, I'm not sure how to prevent this issue.

What do you think?

matthewp commented 7 years ago

We shouldn't need to preload the entire graph, just the portion that the conditional loaded module depends on. This is what happens in steal.js as well, if you think about it. We System.import(main), but when we encounter a conditional module during normalization, that conditional module and its dependencies are executed right away. But the rest of the graph is not.

I don't follow about how the state might change, so let's talk about it if you want.

m-mujica commented 7 years ago

if the app loads locale/#{lang},

the loader will, before the graph executes:

let's say lang looks like this:

module.exports = window.language;

if the loader does the conditional part before the graph is executing ,window.language will most likely be undefined (I guess in this case the loader will throw b/c it won't find the bundle lets say it doesn't)

what if the user has a main.js with:

window.language = "es";

that's what I mean by state change. Does it make sense?

matthewp commented 7 years ago

I do, that's because the lang module actually depends on side-effects of main.js, right? I think what you are describing would be a problem in the regular stealTools.build. As long as lang imports any of its dependencies, there shouldn't be a problem.