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 proposal #285

Closed
guybedford opened this issue Dec 29, 2014 · 58 comments
Closed

Conditional proposal #285

guybedford opened this issue Dec 29, 2014 · 58 comments

Comments

@guybedford
Copy link
Member

Here's a variation that captures the previous features of conditionals (#189, #126), but is much less verbose:

System.sites = {
  // in this case the `./detect/browser/name.js` module has `export default browserName`
  // which is a string, that then gets used as a conditional wildcard in the mapping
  'browserName': './detect/browser/name.js',
  'browser/feature.js': '/some/package/#{browserName}/feature.js'
}

Conditionals would be equally possible to set through paths and map, and work in imports as they are the final normalize action before plugins.

The symbol is #{...} which acts by simple string replacement. Alternative suggestions to # welcome.

Please give feedback! This is the most terrifying API I've ever designed.

@guybedford
Copy link
Member Author

I've updated the proposal to actually make sense.

Note that the nice thing about this proposal is that it could be supported in import statements:

browser-impl.js

import {ua} from 'browser-user-agent';
export var browser = ua.browser;
if (default ua.browser == 'ie' && ua.version < 9)
  browser = 'unsupported';

module.js

import {feature} from './browsers/#{./browser-impl.js}.js';

feature.doStuff();

Having gone around the circle in terms of syntactical conditionals, it seems we've come around again.

Less typing must win here though, and syntax is the least typing.

@guybedford
Copy link
Member Author

In terms of allowing the builder to statically trace the possible conditions, this can be done simply through file system inspection, which was the issue in the original proposal along these lines at #9 (comment).

@guybedford
Copy link
Member Author

The assumption in the above is that condition values never contain / or !, and always have single depth (we could support arbitrary depth, but I can't see that any use case would justify it?).

@matthewp
Copy link
Contributor

I prefer #189. I look at 'another/module#?[./ismodern]' and can't tell what is going on.

@guybedford
Copy link
Member Author

@matthewp it means only load the dependency if the module located at ./ismodern has a default export returning true. Better syntax welcome, but I like the # because it seems the most available character without having to get into escaping.

@matthewp
Copy link
Contributor

I really dislike DSLs inside of strings. I think it both looks ugly doesn't scale past very simple meanings (like the ! for plugins). But if you're going to do it anyways please provide a verbose object-based way of doing conditionals so people like me can still have the feature :-)

As to this proposal, why is # and ? both needed? I still can't tell what # means... With ? I'm assuming the left side is the module and the right side is the condition.

@guybedford
Copy link
Member Author

It's two conditional proposals - the one is for string conditions and the other is for boolean conditions.

I'm using #{...} to indicate substitution of the string condition into the module name that is loaded, because it means we have no escaping issues at all (only assumption being condition modules never have ] in the name).

Then #?... at the end of the name is being used to indicate boolean conditions. The # coming first because that is our 'escape' character effectively.

Simplifications to the above are welcome.

The object design is incompatible with this design unfortunately - as it means listing every possible outcome. The major selling point of the string substitutions is that outcomes are computed, and not listed, hence it saves typing.

@guybedford guybedford changed the title Alternative conditional proposal Conditional proposal Dec 30, 2014
@MajorBreakfast
Copy link
Contributor

I think that code in your comment won't compile since exports statements need to be on the top level #285 (comment) However, it's clear what you mean.

That said, it's one more string DSL - yay :)

An idea for the string interpolation syntax: No hash sign -> nicer, shorter

import '/some/package/[browser-name]/feature.js'
import 'square-brace-\[\]-module' // Escaped

An idea for the boolean syntax:

import '[is-modern]?[fancy-module]:[lousy-module]'
import '[is-modern]?[fancy-module]' // @empty is implict
import '[is-modern]?[app/[browser-name]]:[lousy-module]' // In the future: Boolean and interpolation combined

@guybedford
Copy link
Member Author

Thanks, yes trying to keep the DSL to a minimum. The boolean condition will not have negation or else syntax.

Minor Adjustment for Consideration

It could be useful to assume condition modules will always end in .js, so that conditions become:

promise.js

  // no .js needed for haspromise
  import './promise-polyfill#?./haspromise';
  export Reflect.global.Promise;

The alternative to the above is to have conditions themselves defined in the sites table or contextual map. These two are compatible as we normalize the condition, then if the normalized condition doesn't already end in .js we add the extension.

@guybedford
Copy link
Member Author

Random aesthetic point - which seems better:

import {feature} from 'feature-#{env}.js';

OR

import {feature} from 'feature-#[env].js';

@guybedford
Copy link
Member Author

I'm also tempted to leave out the boolean conditional, because it can always be substituted for the branch conditional, and I've yet to see a scenario that justifies it's use.

The biggest argument for the boolean conditional is probably polyfills, since the boolean conditional can only be used to load side effects (which in general are supposed to be restricted to application code in modular design).

But a polyfill should probably not rely on explicit global assumptions, and instead return the Promise binding for example, which would be wrapped in a polyfill package:

promise-polyfill.js

import 'global-promise-polyfill';
export {Promise};

promise-native.js

export {Promise};

promise package, package.json

main: 'promise-#{./whichpromise}.js';

whichpromise.js

  var type;
  if (typeof Promise != 'undefined' && Promise.all && Promise.race) {
    // perhaps a browser sniff as well to know if we need the polyfill for perf reasons
    type = 'native';
  }
  else
    type = 'polyfill';
  export default type;

So the only use case left is side effects of application code which should only run in certain environments, what one would think of as code like:

app.js

doStuff();
if (feature)
  doOtherStuff();

But the above can always be refactored into:

app-env1.js

doStuff();

app-env2.js

doStuff();
doOtherStuff();

So I don't see a strong argument left for the boolean conditional.

@MajorBreakfast
Copy link
Contributor

import {feature} from 'feature-#{env}.js'

I actually prefer the curlies, because it reminds me of ES6 string templates (with "#" instead of "$"). The familiarity makes it easier to parse. The hash sign is also a good choice because it can"t pop up in an URL since the hash section is client-side.

No boolean condition is fine, I think.

The ".js" file ending should be optional.

@guybedford
Copy link
Member Author

Thanks - great points. I can see curlies working.

@matthewp
Copy link
Contributor

matthewp commented Jan 1, 2015

+1 to es6-like string interpolation.

@marcfallows
Copy link
Contributor

This complicates things slightly (unless it was already planned), but it could also be useful to dig into the value if an object is exported.

System.sites = {
  'featureConfig': './detect/features.js',
  'animate.js': '/some/module/animations/#{featureConfig.animate}.js',
  'design.js': '/some/module/resolutions/#{featureConfig.design}.js'
}
// ./detect/features.js

// Would do feature detection here to choose appropriate config.
export var config = {
   animate: 'css',
   design: 'mobile'
}

@guybedford
Copy link
Member Author

@marcfallows I have considered this actually, but one could just have a folder with separate modules:

features/animate.js
features/design.js

I'm not sure there is that much cost?

Note also that you can set features in a file too:

dev-environment.js

System.set('/features/animate': System.newModule({ default: 'css' });
System.set('/features/design': System.newModule({ default: 'mobile' });

Perhaps we can create a shorthand for the above allowing for easier setting of conditions through configuration:

System.config({
  conditions: {
    'features/animate': true
  }
});

Will just have to see how that works out with the normalization systems.

@marcfallows
Copy link
Contributor

@guybedford My concern with folders with separate modules is that you would need conditional logic in each of the modules.

It would be more ideal if I could make all of the decisions in one module, and then use that information to load only the right modules. Or alternatively I would include Modernizr and then I could also do:

import 'lib/flashcanvas#?{Modernizr.canvas}';

Otherwise I have to start creating a lot of small modules which export a single Modernizr setting.

@guybedford
Copy link
Member Author

@marcfallows this is certainly a possibility. Because we're assuming auto-extensions for the condition module, it could be safe to assume that a . means a condition property so it can work.

@speigg
Copy link

speigg commented Jan 28, 2015

Isn't conditional loading handled by the programmatic api?

import Modernizr from 'Modernizr'
if (Modernizr.canvas) {
   System.import('lib/flashcanvas')
}

It seems a bit silly to add extra syntax when the above is simple enough, but maybe I'm missing something? This also means there are less new things people have to learn. And rather than having a syntax that mimics ES6 template strings, why not just use ES6 template strings?

// assuming env is defined in this scope
System.import(`feature-#{env}`).then(function(feature){
   // ... 
})

@matthewp
Copy link
Contributor

@speigg The problem is that your System.import means that lib/flashcanvas is not part of the module's dependency graph. This makes builds much more difficult.

@speigg
Copy link

speigg commented Jan 28, 2015

@matthewp True. But for any of the dependencies, it is possible that System.import will be used to load something dynamically (it is, after all, part of the spec), so it seems like this is something that will have to be dealt with regardless (unless you are also the author of all your dependencies, which is unlikely, in which case you can be assured that System.import isn't used anywhere).

@speigg
Copy link

speigg commented Jan 28, 2015

Again, maybe I'm missing something, but is it even possible to create a bundle that guarantees to contain all dependencies? I have the impression that when using a bundle, all dependencies will still have to be hosted (like in the development workflow) in order to ensure that everything continues to work, just in case somewhere a dynamic System.import was used.

Perhaps it just considered bad practice to use the dynamic System.import API anywhere but in the root html document, and I am unaware?

@matthewp
Copy link
Contributor

@speigg As long as you create a bundle for every time you use the dynamic System.import you are fine. I use it all the time, for progressive loading. I just make sure that i have a bundle for the modules I am progressively loading.

@speigg
Copy link

speigg commented Jan 28, 2015

But that's just for code you have control over. Is that common practice? What about your dependencies?

@guybedford
Copy link
Member Author

@speigg the idea here is to build up a full static conditional graph where we know what conditions lead to which modules, all in a properly portable / modular way.

System.import tracing is also something that can be done to build up a dynamic loading graph, but the conditional graph is the focus for now as these can always be added manually for tracing.

@guybedford
Copy link
Member Author

@crisptrutski thanks for the feedback. Perhaps import 'lib/flashcanvas#|Modernizr/canvas'; for a full guard?

Conditional Adjustment Proposal

Instead of the condition being a module, have the condition as a member expression

For example:

import {feature} from './browsers/#{./browser-impl.js}.js';

Might become:

import {feature} from './browsers/#{browser.type}.js';

We thus restrict the use of / and . in the condition name itself, so that it has to be mapped (throwing an error for say ./browsers/#{browser/type}.js or ./browsers/#{./browser.js}.js. This way we allow member expressions while keeping the syntax well-defined.

There's a slippery slope here when enabling more syntax, but I think member expressions may well make sense in order to avoid having too many disparate condition modules, which could be difficult to manage.

@guybedford
Copy link
Member Author

Actually the syntax can easily be extended to become arbitrarily powerful via plugins:

import 'q|#{process.env.X}=="Y"!eval'

The above would load the module q only if the process module had env.X matching the condition.

Where an eval plugin could be simply written:

export function translate(load) {
  return 'export default ' + load.name;
}

So we do have full flexibility with this syntax, only in a way that doesn't encourage it's abuse too easily!

I guess the caveat here is that member expressions are supported provided plugin syntax is not present.

Alternatively we assume that the condition is always mapped:

System.map['isdev'] = '#{process.env.NODE_ENV}=="development"!eval';
---
import 'q|~isdev'; // leading ~ for negation in boolean conditional

@MajorBreakfast
Copy link
Contributor

  • I like the pipe (without prepended hash) for the conditional syntax. It reminds me of how the pipe is used in math.
    • \| can be used to escape it
  • I don't like the member expression thing. Module paths are more flexible than access to globals.
System.map['isdev'] = 'process.env.NODE_ENV=="development"!eval' // Alternative for this case: Let eval access the global
  • A pity that ! is already used by the plugin system. It makes more sense for negation. The 0.17 branch already necessitates to add .js everywhere. Changing the plugin system to something else is an option. Especially since we want to drop !css anyway.

@guybedford
Copy link
Member Author

We're weary to introduce a new character that would need to be escaped, instead we're looking at #? on its own again.

Current consensus implemented in the dev branch:

import {feature} from './browsers/#{browser.type}.js';
import 'es5-shim#?~browser.es5';

@guybedford
Copy link
Member Author

@MajorBreakfast note that the member expression feature is not global access - it is a member expression of a module (the first part being the module name). Also plugin syntax is very much confirmed and has a long history at this point.

@guybedford
Copy link
Member Author

Merged into master.

@martinmicunda
Copy link

Hi @guybedford

I have situation in my app where I want to use conditional for backend-less approach (e.g. use $httpBackend mock in angular) but only for test environment.

System.set('env', System.newModule({ 'test': true })); // this value is hardcoded
import EmployeeResource from './employee.resource';
import employeeResourceMock from './employee.resource.mock.js#?env.test'; // this should be only load for test env

The problem is that I would like to set env value dynamically. For example when I am running command ENV=test gulp serve it would set test value to true

ENV=test gulp serve
System.set('env', System.newModule({ 'test': process.env.ENV }));  // this will set to true

and if I don't pass any arguments it would set test to false

gulp serve
System.set('env', System.newModule({ 'test': process.env.ENV }));  // this will set to false

Do you if that's possible with new conditional proposal? Thanks

@guybedford
Copy link
Member Author

@martinmicunda yes this is completely possible in that latest SystemJS. It's just undocumented in the release to get testing feedback on it first.

@martinmicunda
Copy link

@guybedford could you point to right directions how to that as I been looking at test files but I could not figure out how to set env dynamically? Thanks

@guybedford
Copy link
Member Author

What isn't working in your above example?

@martinmicunda
Copy link

@guybedford I am settings the environment in config.js

System.config({
  "baseURL": "./",
  "defaultJSExtensions": true,
  "transpiler": "babel",
  "babelOptions": {
    "optional": [
      "runtime"
    ]
  },
  "paths": {
    "github:*": "jspm_packages/github/*",
    "npm:*": "jspm_packages/npm/*"
  }
});
System.set('env', System.newModule({ 'test': process.env.ENV }));

but I am getting follow error in browser Error: process is not defined which make sense as process doesn't exist in browser. So my question is how to set this value dynamically in config.js when I run

ENV=test gulp serve

or 

gulp serve --env=test

@guybedford
Copy link
Member Author

@martinmicunda it's up to your systems to set an environment global here to manage this. window.env should suffice.

@martinmicunda
Copy link

@guybedford ah got it...

Just one more thing when I use follow code in config.js

System.config({
  "baseURL": "./",
  "defaultJSExtensions": true,
  "transpiler": "babel",
  "babelOptions": {
    "optional": [
      "runtime"
    ]
  },
  "paths": {
    "github:*": "jspm_packages/github/*",
    "npm:*": "jspm_packages/npm/*"
  }
});

System.set('env', System.newModule({ 'test': false })); // --> this cause error

System.config({
  "map": {}
})

and I run jspm install I am getting follow error (NOTE: if I add above System.set() line to my application code the jspm install works fine:

> jspm install


err  TypeError: undefined is not a function
         at Config.eval (eval at <anonymous> (/Users/martinmicunda/projects/groupon/projects/brands_ui-es6/node_modules/jspm/lib/config/loader.js:94:15), <anonymous>:16:26)
         at Config.read (/Users/martinmicunda/projects/groupon/projects/brands_ui-es6/node_modules/jspm/lib/config/loader.js:94:3)
         at /Users/martinmicunda/projects/groupon/projects/brands_ui-es6/node_modules/jspm/lib/config.js:94:26
         at runMicrotasksCallback (node.js:337:7)
         at process._tickCallback (node.js:355:11)
         at Function.Module.runMain (module.js:503:11)
         at startup (node.js:129:16)
         at node.js:814:3

@guybedford
Copy link
Member Author

Yes System.config needs to contain config data only. Use a separate file for code execution.

@guybedford
Copy link
Member Author

I mean jspm config.js...

@martinmicunda
Copy link

@guybedford ok I am passing variable through gulp-inject into html as set them as global.

        <!-- build:remove -->
        <script src="jspm_packages/system.js"></script>
        <script src="jspm.conf.js"></script>
        <script>
            System.import('app/bootstrap').catch(console.error.bind(console)); // make sure any errors are print to console
        </script>
        <!-- endbuild -->

        <script type="text/javascript">
            window.ENV = {
                <!-- inject:env -->
                mock: true,
                environment: 'test'

                <!-- endinject -->
            };
            System.set('ENV', System.newModule({ 'default': window.ENV, __useDefault: true })); // it requires for conditional ES6 module loader
        </script>

app code

import EmployeeResource from './employee.resource';
import employeeResourceMock from './employee.resource.mock.js#?ENV.mock';

and it works fine for my development flow however when I try use systemjs-builder in my production flow I am getting follow error:

Error: Error: ENOENT, open '/Users/martinmicunda/projects/groupon/projects/brands_ui-es6/ENV.js'

I am guessing that global variables are not available as I defined them in html. So my first question is if systemjs-builder support conditional import and if yes how do you pass variable through systemjs-builder to application?

    var Builder = require('systemjs-builder');
    var builder = new Builder();

    builder.loadConfig('./jspm.conf.js')
        .then(function() {
            builder.buildSFX('src/app/bootstrap', paths.tmp.scripts + 'build.js', { sourceMaps: true, config: {sourceRoot: paths.tmp.scripts} })
                .then(function() {
                    return cb();
                })
                .catch(function(ex) {
                    cb(new Error(ex));
                });
        });

@martinmicunda
Copy link

@guybedford did you have chance to look at this? Thanks

@guybedford
Copy link
Member Author

There is no support yet for conditional builds, which is also one of the reasons conditionals aren't documented currently.

I'd suggest setting globals directly if you can for builds, although in effect this workflow may just end up being equivalent to having a variable map config anyway.

Sorry I can't be of more help than that right now.

@martinmicunda
Copy link

hmm that didn't work.. I found temporary solution for my production build (I am using coditional imports only in dev and test env). Basically I am using gulp-preprocess task before I build a bundle and preprocess task remove mock code from my files. It would be nice when systemjs-builder will support conditional imports and also option to pass globals in the future but I understand this my take a while...

//@exclude
import employeeResourceMock from './employee.resource.mock.js#?ENV.mock';
//@endexclude

@martinmicunda
Copy link

Hi @guybedford. Do yo know if there is plan to add support for conditional module syntax to systemjs-builder in near future? Thanks

@guybedford
Copy link
Member Author

@martinmicunda this is certainly on the roadmap, but implementation is not currently being prioritised.

@omerts
Copy link

omerts commented Aug 10, 2015

how do you load the conditional loader from a relative path?
using

import fetch from '#{./FetchSelector.js}';

throws:

Condition modules cannot contain . or / in the name.

@guybedford
Copy link
Member Author

@omerts condition modules need to be mapped via map configuration so they don't have . or / in the name.

@omerts
Copy link

omerts commented Aug 10, 2015

@guybedford thank you :)

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

9 participants