Conditional support for static bundle build

@guybedford

For last couple of months I am using conditional import feature in my app and I can see you have added this feature to builder but it doesn't work as I would expect so let me explain my use case.

Development Workflow:
I have file called env.conditions.js where I defined global conditions (these value are always injected from cli e.g. by gulp-inject):

window.ENV = {
    <!-- inject:env -->
    mock: false, optimize: false, environment: 'dev',
    <!-- endinject -->
};

System.set('ENV', System.newModule({ 'default': window.ENV, __useDefault: true }));

then in my index.html I include this file:

<script src="jspm_packages/system.js"></script>
<script src="jspm.conf.js"></script>
<script src="env.conditions.js"></script>
<script>
    System.import('app/app').catch(console.error.bind(console)); 
</script>

here is how I use these conditions in my code:

import './config.#{ENV|environment}.js';
import './employee.mock.js#?ENV|mock';

and all this work great.

SFX bundle Workflow:
Currently I am using gulp-preprocess to remove all these conditions from source code to create bundle and this is really ugly workaround hack and I saw couple days ago you have added conditional support to builder so I decided to try it but I can't get conditional import works with builder in way that I need for my project. I know that current builder conditional support only condition for different packages but my requirement is that builder load env.conditions.js file that includes all these conditions and build the SFX bundle. When I try build the bundle then builder is throwing error because it can't find conditions that are defined in the source code. I also tried add these conditions to source code but nothing work for me so is there way that I can include this file somewhere in build config so when builder start creating bundle it first load these conditions?. Thanks

gulp.task('bundle', (cb) => {
    const Builder = require('systemjs-builder');
    const builder = new Builder();
    const inputPath = 'src/app/app'; 
    const outputFile = `${path.tmp.scripts}build.js`;
    const outputOptions = { sourceMaps: true, config: {sourceRoot: path.tmp.scripts} };

    builder.loadConfig(`${path.root}/jspm.conf.js`)
        .then(() => {
            builder.buildStatic(inputPath, outputFile, outputOptions)
                .then(() => cb())
                .catch((ex) => cb(new Error(ex)));
        });
});

@martinmicunda

@martinmicunda thanks for trying this work out!

SFX bundles do support conditional builds, but they need to be provided with the exact environment conditions to build.

This can be done via -

buildStatic(inputPath, outputFile, { conditions: { 'ENV|mock': true, 'ENV|environment': 'production', 'ENV|optimize': true } })

The builder will need a complete unique conditions solution to do an SFX build (unlike normal bundling which will be able to deal with conditions being indeterminate). Tracking of SFX build support for indeterminate conditions is in #311.

Also I guess you know but just to mention again this stuff isn't completely stable yet.

@guybedford

@guybedford hmm that doesn't work ... Am I missing something?

this is my SFX builder task:

gulp.task('bundle', ['jshint', 'js-preprocess'], (cb) => {
    const Builder = require('systemjs-builder');
    const builder = new Builder();
    const inputPath = 'src/app/app'; with conditional import for systemjs-builder will be fixed
    const outputFile = `${path.tmp.scripts}build.js`;
    const outputOptions = {conditions: { 'ENV|mock': false, 'ENV|environment': 'prod', 'ENV|optimize': true }, sourceMaps: true, config: {sourceRoot: path.tmp.scripts}};

    builder.loadConfig(`${path.root}/jspm.conf.js`)
        .then(() => {
            builder.buildStatic(inputPath, outputFile, outputOptions)
                .then(() => cb())
                .catch((ex) => cb(new Error(ex)));
        });
});

and these are conditions in my source code:

import './config.#{ENV|environment}.js';
import './employee.mock.js#?ENV|mock';

I am getting follow error:

 Error: Error: ENOENT, open '/Users/martinmicunda/projects/employee-scheduling/ui/ENV.js'
    at /Users/martinmicunda/projects/employee-scheduling/ui/gulp/tasks/scripts.js:68:35
    at lib$rsvp$$internal$$tryCatch (/Users/martinmicunda/projects/employee-scheduling/ui/node_modules/systemjs-builder/node_modules/rsvp/dist/rsvp.js:493:16)
    at lib$rsvp$$internal$$invokeCallback (/Users/martinmicunda/projects/employee-scheduling/ui/node_modules/systemjs-builder/node_modules/rsvp/dist/rsvp.js:505:17)
    at lib$rsvp$$internal$$publish (/Users/martinmicunda/projects/employee-scheduling/ui/node_modules/systemjs-builder/node_modules/rsvp/dist/rsvp.js:476:11)
    at lib$rsvp$$internal$$publishRejection (/Users/martinmicunda/projects/employee-scheduling/ui/node_modules/systemjs-builder/node_modules/rsvp/dist/rsvp.js:419:7)
    at lib$rsvp$asap$$flush (/Users/martinmicunda/projects/employee-scheduling/ui/node_modules/systemjs-builder/node_modules/rsvp/dist/rsvp.js:1198:9)
    at process._tickCallback (node.js:355:11)

This sounds like it could be a bug due to the handling of environment condition canonicalization not detecting the set module correctly. I'll look into it!

@guybedford

@guybedford thanks .. You can ping me once you fix it and I will verify if it's working

Sure, will do!

@martinmicunda

@martinmicunda I just had a look at this and the reason is that the builder will still try to trace the environment conditional itself as it is considered as a dependency to include in the bundle, as conditions are always evaluated dynamically at runtime, and not built in statically.

@guybedford

@guybedford so how can include 'env.conditions.js' as a dependency into bundle should be that done through systemjs config?

@martinmicunda

@martinmicunda it will be done for you - you just need to ensure that the condition is itself a module that is available to be loaded. I've added a test in 55b4198 which may also help to see how it works.

@guybedford

@guybedford I still didn't get that fully running but I made at least some progress...

env.conditions.js -> where I define ENV module

'use strict';

window.ENV = {
    mock: true, optimize: false, environment: 'test'
};

System.set('ENV', System.newModule({ 'default': window.ENV, __useDefault: true }));

jspm.config.js -> I include this file in map block

  map: {
      "ENV": "env.conditions.js",

gulp SXF bundle task

gulp.task('bundle', ['jshint'], (cb) => {
    const ENV = !!util.env.env ? util.env.env : 'DEV';
    const Builder = require('systemjs-builder');
    const builder = new Builder();
    const inputPath = 'src/app/app'; 
    const outputFile = `${path.tmp.scripts}build.js`;
    const outputOptions = { sourceMaps: true, config: {sourceRoot: path.tmp.scripts}, conditions: { 'ENV|mock': ENV.toLowerCase() === 'test', 'ENV|environment': ENV.toLowerCase()} };

    builder.loadConfig(`${path.root}/jspm.conf.js`)
        .then(() => {
            builder.buildStatic(inputPath, outputFile, outputOptions)
                .then(() => cb())
                .catch((ex) => cb(new Error(ex)));
        });
});

this will create SXF bundle without error but when I load my app I am getting follow error:

Uncaught ReferenceError: System is not defined

I am assuming it something to do with System.set that I define in env.conditions.js file...

I have also one question why do I need to still specify conditions: { 'ENV|mock': ENV.toLowerCase() === 'test', 'ENV|environment': ENV.toLowerCase()} } in output options when I already include them to jspm.config.js map block map: {"ENV": "env.conditions.js",...} as module. Wouldn't be easy when I specify all my conditions in one file and then builder just load them as module so I don't need to duplicate this logic?

@martinmicunda

@martinmicunda try writing your condition module as just:

env.conditions.js

export var mock = true;
export var optimize = true;
// ..etc..

with a System.config({ map: { 'ENV': 'env.conditions.js' } }).

@martinmicunda

@martinmicunda it sounds like this may not be linked in correctly, because otherwise the config normalization problem you have in #339 (comment) would have been resolved by the latest commit.

I'd suggest waiting for the release and trying again then.

@guybedford

@guybedford I will try that.. I already wrote a blog post about Conditional module loading with SystemJS (I am just waiting till I get run builder condition and then I will publish it).

@martinmicunda

@martinmicunda @guybedford I suspect a bad normalisation of the condition, and this is based of the trace output. Let me explain, based on the above example, if you check the returned tree of the trace method of the builder, you'll see that for employee.mock entry it looks like:

'src/app/core/resources/employee/employee.mock.js#?src/app/core/config/env.conditions.js|mock.js'

So, those extra .js last characters at the end make the difference, therefore a quick fix is to add 'src/app/core/config/env.conditions.js|mock.js': false, to the conditions object to eliminate the mocks from the prod build. A quick hack, I know, but at least it works as expected, in my case at least.

EDIT: Fix typo.

@lucianlature

@lucianlature yeah I can confirm when I add extra .js it fix the problem.

conditions: { 'src/app/core/config/env.conditions.js|mock.js': false, 'src/app/core/config/env.conditions.js|environment': 'prod'}

It would be nice in the future if we would not need to use src/app/core/config/env.conditions.js path but instead ENV variable as this path is already specify in jspm.config.js so if I want to change the path I need to make changes in two places now.

The below is separate error and not related to conditions:
@guybedford I been testing my app with JSPM v0.16.13 and SystemJS Builder v0.14.8 and everything works fine however when I update SystemJS Builder to v0.14.9 my bundle is broken and I can see lot of errors in my browser console like this:

TypeError: Cannot read property '1' of null
    at update (http://localhost:8000/scripts/main-b899601eb0.min.js:16703:49)
    at link (http://localhost:8000/scripts/main-b899601eb0.min.js:16720:13)

the cause of this problem is that uiSrefActive is undefined

return {
          restrict: 'A',
          require: ['?^uiSrefActive', '?^uiSrefActiveEq'],
          link: function(scope, element, attrs, uiSrefActive) {
                  var activeDirective = uiSrefActive[1] || uiSrefActive[0];

I been debugging this issue but can't find real casue of this problem but I believe it had to be introduced in SystemJS Builder v0.14.9 as my development use latest JSPM that use latest SystemJS v0.19.5 and I can't see these errors so I assume the issue is with builder v0.14.9

@martinmicunda

@martinmicunda can you share a test repo for this at all?

@guybedford

@guybedford here is the repo

I have updated repo with latest JSPM v0.16.13 and SystemJS Builder v0.14.8 this morning and everything works great included conditional imports. However when I update SystemJS builder to v0.14.9 I can see above error.

Steps to reproduce error:

git clone git@github.com:martinmicunda/employee-scheduling-ui.git 
cd employee-scheduling-ui

Update SystemJS builder to v0.14.9 in package.json and run follow commands:

npm install
npm start -- --env=TEST --optimize=true

npm start should automatically open your browser (open browser console and observe the error):

TIP: you might disable uglify option for bundle in gulp/tasks/build.js

@martinmicunda

@martinmicunda thanks I had a look at this and I suspect this may be due to #367.

@guybedford

@guybedford ok.. I will keep eye on #367 and once it fixed I will run my tests...

@guybedford

@guybedford this was a fun thread. helped my get my env setup, but Im having an issue with the current version. keep getting an issue like [TypeError: No resolution found at all for condition data/initial-states.#{ENV.production.js|environment}.js.] when trying to buildStatic. Also, it does not seem to take the config set in the conditions property and is looking for and ENV file.

@wprater

@wprater condition names need to match up carefully. Did you try specifying the condition name as exactly "ENV.production.js|environment": "x"?

@guybedford

@guybedford adding the .js did the trick, however, what if I wanted to instead use another file for ENV during my build instead of the conditions prop? I still get the same error unless I pass the conditions prop. Or is that a requirement?

Im importing like import * as InitialStates from '../data/initial-states.#{ENV|environment}';

also, where is the interpolation syntax for #MAPPING|propertyName defined? I see a couple different uses in this thread.

@wprater

@wprater I would advise using a path configuration to set the ENV module:

builder.config({
  paths: {
    'ENV': 'ENV.production.js'
  }
});

That way you can alter the environment module at any point, and also then you only need to refer to it as "ENV|environment": "x" in the conditional system.

I've created #426 to try and avoid these normalization issues though in general (and allow map to work properly here) as it sounds like a common catch so far.

The conditions object is a requirement for SFX builds, but isn't mandatory for normal bundles. This is because SFX builds need to exactly link to a single tree resolution.

The syntax itself is mostly undocumented until it is deemed stable. There are some notes in the code at https://github.com/systemjs/systemjs/blob/master/lib/conditionals.js.

@guybedford

@guybedford that's exactly what I had tried before, but there was still an issue. Maybe it's a bug?

[Error: ENOENT: no such file or directory, open '/Users/wprater/Development/xxx-yyy/src/ENV.production.js']

maybe I still don't understand how this conditional system works. I'll review the code. Seems I could use the paths to not provide the conditions object.

That way you can alter the environment module at any point, and also then you only need to refer to it as "ENV|environment": "x" in the conditional system.

I don't understand this bit?

@wprater

@wprater you still need to provide the conditions object with paths, the path just allows the object to read conditions: { "ENV|environment": "x" } instead of conditions: { "ENV.production.js|environment": "x" }. Paths config should definitely work to provide that name simplification though.

yeah, they were not working for me, which is why I wanted to post here again to make sure Im doing everything correctly. It would be nice to have a ENV.development.js and an ENV.production.js and just override the path in the build process, but since I need to specify all the conditions again, there is no need for me to have these two fields and alter the path at all. So I've worked around this by having a separate config file, keeping the path and just changed the environment condition in the build process.

Navigation Menu

Repository navigation

Conditional support for static bundle build #339

10 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Conditional support for static bundle build #339

Description

Activity

guybedford commented on Oct 5, 2015

martinmicunda commented on Oct 5, 2015

guybedford commented on Oct 5, 2015

martinmicunda commented on Oct 5, 2015

guybedford commented on Oct 5, 2015

guybedford commented on Oct 8, 2015

martinmicunda commented on Oct 8, 2015

guybedford commented on Oct 8, 2015

martinmicunda commented on Oct 9, 2015

guybedford commented on Oct 9, 2015

10 remaining items

guybedford commented on Oct 17, 2015

martinmicunda commented on Oct 19, 2015

lucianlature commented on Oct 22, 2015

martinmicunda commented on Oct 23, 2015

guybedford commented on Oct 23, 2015

martinmicunda commented on Oct 23, 2015

guybedford commented on Oct 27, 2015

martinmicunda commented on Oct 27, 2015

wprater commented on Dec 10, 2015

guybedford commented on Dec 10, 2015

wprater commented on Dec 10, 2015

guybedford commented on Dec 11, 2015

wprater commented on Dec 11, 2015

guybedford commented on Dec 12, 2015

wprater commented on Dec 15, 2015

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions