Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conditional loading #9

Closed
guybedford opened this issue Aug 8, 2013 · 26 comments
Closed

Conditional loading #9

guybedford opened this issue Aug 8, 2013 · 26 comments

Comments

@guybedford
Copy link
Member

Outside the scope of the current plugin system... perhaps consider a natively supported loading syntax for conditionals like:

jspm.import('some/condition ? some/module : another/module');
jspm.import('!some/condition ? some/module');
jspm.import('some/condition ? some/module, another/module');

For inline conditions, these can still be provided by:

jspm.set('some/condition', new Module({ default: someInlineFunction() });
@guumaster
Copy link

Why would you do this in the import declaration? It doesn't seem like a good idea.

@guybedford
Copy link
Member Author

It has to be handled at the loader level. This is one suggestion, based on the RequireJS plugin, https://github.com/guybedford/require-is.

But yeah it is not ideal.

Effectively conditional loading is conditional normalization based on a feature flag. Perhaps simply using conditional map configuration would be enough, but there is definitely a need to provide this workflow somehow.

@juandopazo
Copy link

FWIW I'm working on something that looks like this:

System.triggers.foo = {
  test: function () { /* do some feature test */ return true },
  replaceWith: 'bar'
};
System.import('foo').then(function (result) {
  // result is actually 'bar'
});

@guybedford
Copy link
Member Author

Interesting - what is your use case for this?

My worry with configuration-based approaches is that we impose configuration injection for packages. If I have a package that uses conditional loading (say for browser code variations), then I can't easily share my package with you, without having you manually inject configuration.

My other worry is that conditional loading affects the static dependency tree. And for tracing, it is very important that the dependency tree can always be easily analysed. By using a syntax for conditionals, a tracer can also understand and follow that syntax, allowing for conditional subtrees to be traced properly.

Would be very interested to hear your thoughts further though.

@juandopazo
Copy link

Main use case simplified: I have a "graphics" module which can be implemented in SVG, VML or Canvas. I want to get the correct implementation based on the browser.

I don't think there's a way around configuration for this case. What would some/condition look like? If it had to point to a JS value or a function, it'd have to be stored somewhere previously. Or is it a module itself?

@guybedford
Copy link
Member Author

Yeah it's a module itself containing a feature test returning true or false.

When running a trace / build, one would then have to manually determine which feature branches to include.

If features were used like public modules though (having a common 'IE8' condition, etc, that all packages share), then it could become a reasonable approach. Still working it out.

@guybedford
Copy link
Member Author

First proposal (previous variations deleted):

app.js

  import { method } from './method-all-platforms#';

method-all-platforms.js

  export default './detect';
  export var mobile = './module-mobile';
  export var tablet = './module-tablet';
  export var desktop = './module-desktop';

detect.js

  var platform;
  if (...)
    platform = 'mobile';
  else if (...)
    platform = 'tablet';
  else if (...)
    platform = 'desktop';
  export default platform;

@guybedford
Copy link
Member Author

Second Proposal (preferred):

Surely we're always going to have the implementations under a simple variation anyway (method-browser, method-server)? Then instead of manually declaring each variation, why not just have the detection module choose from a predefined suffix set:

  import { method } from './method-[platform] | server browser mobile';

We've indicated that we should load from ./method-..., where the options are server, browser and mobile, and we've indicated the feature detection module name to use (platform being a module name).

All the information is now in one place with no separate feature module and it's the easiest to read.

The limitation is if we don't have the same pattern for all features. But I think this might be sensible since we require the same export pattern anyway, so the same path pattern can correspond with that fine.

The last option should probably be a catch-all, or we just let the reference error happen.

This does fall over for large sets like languages. We could optionally allow the restriction set itself to be a module for this case:

  module greetings from './i18n/[lang]/greeting | [lang/supported]';

lang.js

  export default navigator.language

lang/supported.js

  export default ['en-US', ...];

@juandopazo
Copy link

I like the approach, but I'm still not convinced from a performance point of view. If I want to hide the use of the condition from the user I need to do this:

// graphics module
import { foo, bar } from './graphics-[implementation] | svg canvas vml';
export { foo, bar }

// page
import { foo } from 'graphics';

Even if I bundle all my condition modules or I insert them in the page using <module> I still need to fetch graphics first, figure out its dependency and go fetch the other implementation, resulting in two requests. With configuration I can do only one request.

@guybedford
Copy link
Member Author

@juandopazo thanks for the feedback, really helps!

The cost of modular configuration is an extra request. I would write it as:

export * from './graphics-[implementation] | svg canvas vml';

I also regularly do this for version forwarding, say if I want to load jquery:

jquery.js

export * from 'jquery@2.1.0';

So this is modular map config in a similar way.

But we solve the problem of this extra module through bundling and multiplexing at the connection level.

@juandopazo
Copy link

@guybedford I forgot export * FromClause was still supported!

I guess you could bundle the graphics module alongside the module that resolves the condition and that would be similar to using configuration ahead of time. I'm starting to like the idea. I'll see if I can implement it during the weekend.

@guybedford
Copy link
Member Author

@juandopazo note that SystemJS optimizes export * from '...'; module loading to work without needing compilation or Traceur (https://github.com/systemjs/systemjs/blob/master/lib/system-formats.js#L64).

Exactly, this can be done either with System.register, System.define or protocol push.

If you're working on it in your own codebase would be great to have a link to it so I can copy across too if you don't mind! If you're working on a fork, a pull request is also very welcome.

@guybedford
Copy link
Member Author

@juandopazo is this still making sense? Or does the API miss something?

@juandopazo
Copy link

I couldn't find the time to write it during the weekend.

We had a couple of discussions about this within our team. Here's what came up.

  • There's reticence against magic module names or encoding information in the FromClause string.
  • Even though encoding the condition in the module name does get around injecting configuration, it still needs the conditional-loading functionality to be injected into the Loader. This can't be done currently.
  • We should author modules in a way that they don't need anything other than the stock Loader to work.
  • Configuration wins in terms of performance.

@caridy
Copy link

caridy commented Apr 8, 2014

@guybedford we have discussed this API extensively, and for us this is a no-go, I will try to articulate some of the thoughts from our discussions:

  • a System.import() statement is equivalent to adding a module tag to the page (in the future).
  • ES modules are statically fact that it is statically verifiable, and abusing the FromClause to import modules conditionally will make those modules unverifiable.
  • when importing a module, it is a conscience decision to import a very specific module, why should a condition be applied there? in theory you will import app top level modules rather than conditional dependencies.
  • modules should not know about conditions, otherwise it will break the contract between module author and consumer of the module. The consumer should BE responsible for provisioning all dependencies based on the module contract.
  • conditions are subject to change overtime (new browsers, new features, etc) while module implementation is set to remain inmutable.
  • the module should work the same with default System than a patched custom module instance.

@caridy
Copy link

caridy commented Apr 8, 2014

oh, ok, @juandopazo beat me here. hopefully this will be enough to backpedal on this proposal.

@guybedford
Copy link
Member Author

@caridy thanks for clarifying, good to hear both descriptions too.

I think the fundamental difference here is configuration. In my experience, configuration is what creates friction for installing packages, so I have heavily designed the loader from the core to work without configuration (but still allow it). I really do think it's important for true modularity. The easier I can get going with your package in my project, the more likely I am to use it, and configuration is the biggest cause of friction.

I do think it is a very important consideration, and one of the key reasons why so many users have moved away from AMD towards Browserify is because of this.

All the other technical arguments can be individually discussed, but the above argument is really fundamental for me.

@caridy
Copy link

caridy commented Apr 8, 2014

@guybedford that can be achieve thru abstraction by framework developers IMO.

@guybedford
Copy link
Member Author

@caridy can you clarify?

@guybedford guybedford mentioned this issue Apr 12, 2014
@justinbmeyer
Copy link
Contributor

Most of the conditional loading we do nowadays are polyfills. We currently handle that using the deps config in config.js.

For instance, config.js might look like:

var supportsUnknownElements = (function(){ /* TEST CODE */ return bool });

if(!supportsUnknownElements){
  System.meta["a-plugin-that-needs-html5"] = {deps: ['html5shiv'], packaged: false};
  System.meta["another-that-needs-html5"] = {deps: ['html5shiv'], packaged: false};
}

{packaged: false} is something we use to make sure a module is not included in the production build (but will still be loaded).

This is one reason why deps should be supported outside the global format.

@guybedford What other use cases are you thinking this is for?

@justinbmeyer
Copy link
Contributor

After re-reading your last comment ... yes, it would be amazing if a module can specify its own dependencies. I like you first proposal. The "a-plugin-that-needs-html5" implemented like:

@import "html5 ? html5shiv";
document.createElement("crazy");

And html5 like:

var a = document.createElement('a');
a.innerHTML = '<xyz></xyz>';

export default var supportsChildNoes = a.childNodes.length == 1 || (function () {
    // assign a false positive if unable to shiv
    (document.createElement)('a');
    var frag = document.createDocumentFragment();
    return (
        typeof frag.cloneNode == 'undefined' ||
            typeof frag.createDocumentFragment == 'undefined' ||
            typeof frag.createElement == 'undefined'
        );
}());

It seems very straightforward. Shivs could provide their own "test" module.

@juandopazo
Copy link

Just to clarify what Caridy didn't answer. We should author our modules so that they can be used without configuration or extra code from the loader. From that stepping point we can then extend the Loader to use configuration to improve the performance of these modules by using conditional loading.

Drawing back from the graphics example, the main graphics module would look like this:

import GraphicsSVG from 'graphics-svg';
import GraphicsVML from 'graphics-vml';

var graphics = doesTheBrowserSupportSVG() ? GraphicsSVG : GraphicsVML;

// Too bad there doesn't seem to be a way to use export * here :(
export default graphics;

This way, even though we're loading a lot of code we don't need, the graphics module will work in any environment that has the ES6 Loader, even those without System-js or anything similar to it. Then we can use a loader extension like System-js to use configuration data to rewrite the graphics module as a dynamic module that knows how not to load the the undesired implementation based on the feature test.

@guybedford
Copy link
Member Author

@juandopazo thanks for clarifying. Yeah that makes sense.

My worry is that any user would still have to have this extension (and its build counterpart) for code to work in production. So it is only a pretend compatibility.

Also I do worry about creating code differences between development and production, as it increases the likelihood of production bugs.

How exactly do you see this rewriting working? Is it something like a pragma approach here?

@juandopazo
Copy link

I don't think there's a single better way to do it. I'm just doing name aliasing for now. If a test passes, I replace the name of the module for the another.

@guybedford
Copy link
Member Author

Yeah this is one of those divisive issues unfortunately. That approach is solid and simple. I would do it myself, but I'm really averse to configuration as if you consider an application with 20 dependencies, all with conditional polyfills, the workflow becomes a configuration nightmare.

I really like the import "es6-promise?./needs-polyfill"; approach for these reasons as it encapsulates all this configuration within the natural modularity.

@guybedford
Copy link
Member Author

@caridy @juandopazo I've finally come around to your thinking on this... I agree a conditional syntax takes us away from being able to statically check imports, and it doesn't really solve this properly.

I think we need to think in terms of environment as you say.

I'm replacing this issue with this one - #126.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants