-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Comments
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. |
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). |
The assumption in the above is that condition values never contain |
I prefer #189. I look at |
@matthewp it means only load the dependency if the module located at |
I really dislike DSLs inside of strings. I think it both looks ugly doesn't scale past very simple meanings (like the As to this proposal, why is |
It's two conditional proposals - the one is for string conditions and I'm using
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. |
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 |
Thanks, yes trying to keep the DSL to a minimum. The boolean condition will not have negation or else syntax. Minor Adjustment for ConsiderationIt could be useful to assume condition modules will always end in 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 |
Random aesthetic point - which seems better: import {feature} from 'feature-#{env}.js'; OR import {feature} from 'feature-#[env].js'; |
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-polyfill.js import 'global-promise-polyfill';
export {Promise}; promise-native.js export {Promise}; promise package, package.json
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
So I don't see a strong argument left for the boolean conditional. |
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. |
Thanks - great points. I can see curlies working. |
+1 to es6-like string interpolation. |
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'
} |
@marcfallows I have considered this actually, but one could just have a folder with separate modules:
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. |
@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. |
@marcfallows this is certainly a possibility. Because we're assuming auto-extensions for the condition module, it could be safe to assume that a |
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){
// ...
}) |
@speigg The problem is that your System.import means that |
@matthewp True. But for any of the dependencies, it is possible that |
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 Perhaps it just considered bad practice to use the dynamic |
@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. |
But that's just for code you have control over. Is that common practice? What about your dependencies? |
@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. |
@crisptrutski thanks for the feedback. Perhaps Conditional Adjustment ProposalInstead 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 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. |
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 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 |
System.map['isdev'] = 'process.env.NODE_ENV=="development"!eval' // Alternative for this case: Let eval access the global
|
We're weary to introduce a new character that would need to be escaped, instead we're looking at Current consensus implemented in the dev branch: import {feature} from './browsers/#{browser.type}.js';
import 'es5-shim#?~browser.es5'; |
@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. |
Merged into master. |
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=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 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 |
@martinmicunda yes this is completely possible in that latest SystemJS. It's just undocumented in the release to get testing feedback on it first. |
@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 |
What isn't working in your above example? |
@guybedford I am settings the environment in
but I am getting follow error in browser
|
@martinmicunda it's up to your systems to set an environment global here to manage this. |
@guybedford ah got it... Just one more thing when I use follow code in 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
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 |
Yes System.config needs to contain config data only. Use a separate file for code execution. |
I mean jspm config.js... |
@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 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 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));
});
}); |
@guybedford did you have chance to look at this? Thanks |
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. |
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 //@exclude
import employeeResourceMock from './employee.resource.mock.js#?ENV.mock';
//@endexclude |
Hi @guybedford. Do yo know if there is plan to add support for conditional module syntax to |
@martinmicunda this is certainly on the roadmap, but implementation is not currently being prioritised. |
how do you load the conditional loader from a relative path? import fetch from '#{./FetchSelector.js}'; throws:
|
@omerts condition modules need to be mapped via map configuration so they don't have |
@guybedford thank you :) |
Here's a variation that captures the previous features of conditionals (#189, #126), but is much less verbose:
Conditionals would be equally possible to set through
paths
andmap
, 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.
The text was updated successfully, but these errors were encountered: