Allow array of paths to the preprocessCss phase

[tstirrat]

Allow a hash of files in outputPaths.app.css to be passed to the CSS preprocessor.

var app = new EmberApp({
    outputPaths: {
      app: {
        css: {
          'main': '/css/main.css',
          'theme/a': '/css/theme/a.css'
        }
      }
    }
  }
});
module.exports = app.toTree();

// app/styles/main.scss    -> dist/css/main.css
// app/styles/theme/a.scss -> dist/css/theme/a.css

Ensures backwards compatibility so that when no paths are configured; app/styles/app.scss will still be written to assets/<app name>.css

Closes #1656.

[tstirrat]

Note: Due to the way the style plugin registry works, the only way to activate SASS in the tests was to "install" one of the following:

registry.add('css', 'broccoli-sass', ['scss', 'sass']);
registry.add('css', 'broccoli-ruby-sass', ['scss', 'sass']);

I chose to mock broccoli-sass in the text fixtures. If you prefer, to avoid confusion, I could probably make an ember addon that registers a new scss interpreter instead:

registry.add('css', 'super-sass', ['scss']);

.. and then the fixtures would contain node_modules/super-sass instead of broccoli-sass

Let me know if this is worth doing.

[MichaelVdheeren]

A good solution for #1656 and thus an alternative for #1816

[rwjblue]

Can you make this multiline please?

[rwjblue]

I left a couple of comments, but this looks good. Thank you for the tests (mocking in that fixture seems good to me for now).

[tstirrat]

I quite like the solution by @MichaelVdheeren in #1816, where it auto builds any non _ prefixed files. This is what is documented by SASS and how sprockets and the sass watch command operate.

The only downside for this is that you do not have specific control over exactly which files are built, and (for my specific needs) you cannot control which files are built under certain environments. e.g. I set a really tiny scss file to be built in test mode to save time and only one theme to be built in dev mode (because broccoli-ruby-sass is slowwww):

var themes = ['themes/a/theme'];

// use a much faster css in test mode
if (process.env.EMBER_ENV === 'test') {
  themes = ['tiny'];
}

if (process.env.EMBER_ENV === 'production') {
  themes = themes.concat([
    'themes/b/theme',
    'themes/c/theme',
    'themes/d/theme',
    'themes/e/theme',
    'themes/f/theme'
  ]);
}

// later:
  preprocessCss: {
    options: {
      paths: themes
    }
  },

I wonder if we could use a combination of this PR and #1816. Perhaps default to 'auto' mode, which builds all non partial scss files; and provide the option of passing an array of paths (or a glob pattern) to be more specific:

preprocessCss: {
  paths: 'auto' // default .. builds anything without a partial _* prefix
},

// or

preprocessCss: {
  paths: ['a', 'b', 'c']
}

This could work in a predictable way for most users, and allow explicit configuration for those who need more.

Ideally, we don't want to decide on one way too hastily and then have to introduce breaking changes later. So your thoughts are welcome.

Naturally, documentation is the key. I'm willing to write it, once we know what we are implementing.

[tstirrat]

one thing to note is that if we do use an auto mode, if people forget to name their partials with the _ prefix, it could start to deteriorate their build performance without them realising why.

[MichaelVdheeren]

Build performance is indeed the biggest problem as I found that using #1816 results in slow builds when you have multiple sass files (in our case for different internet explorers) which import ie bootstrap sass.

[tstirrat]

There's even more to it than just the amount of files you're building. The .sass-cache dir is completely ineffective because sass (ruby) uses the file's path in the key for the cache. each time you run ember build the broccoli temp trees have new names, thus no cache. This is likely also the case for broccoli-sass (node-sass)..

I'm brainstorming ideas on how to get the sass cache to actually work.

It seems selectively including files is the way to go, until there are some perf improvements on broccoli-rub-sass (or node-sass can build extended selectors )

We can always add the auto option at a later date.

[tstirrat]

@rwjblue I've updated the code based on your comments, let me know if there's anything else.

[tstirrat]

one last thing. should we use files: [] instead of paths: []? Looking through ember-cli, I'm thinking that files might be the most consistent.

[rwjblue]

I think this needs to be updated per recent changes app.options.outputPaths.css specifically.

[tstirrat]

@rwjblue rebased and updated to work with the outputPaths change. Tests passing. @jayphelps perhaps you can have a quick look, too.

[tstirrat]

Yeah, I think it makes sense to combine them.

My first inclination is that configuring the "inputs" inside your outputPaths hash feels a little strange. I tried a few other variations and most of them felt worse. The best I can find is if we change outputPaths to paths:

var app = new EmberApp({
    paths: {
      app: {
        css: {
          'main': '/css/main.css',
          'theme/a': '/css/theme/a.css'
        },
        js: '/js/app.js'
      },
      vendor: {
        css: '/css/vendor.css',
        js: '/js/vendor.js'
      },
      testSupport: {
        css: '/css/test-support.css',
        js: '/js/test-support.js'
      }
    }
});

[jayphelps]

@tstirrat LGTM... (except this.name is undefined there btw...I think?)

@rwjblue will prolly have an opinion 🍭

The more I thought about it, the more I think we should stick as close to existing conventions as possible, the most prominent being indeed being grunt. For reference, there are many variations supported by grunt itself, most of which you can see here. We may want to at least consider utilizing the same code that grunt uses under the hood.

[bcardarella]

Needs a rebase

[tstirrat]

I'm happy to go with the merging into outputPaths. I will sort it out this evening.

[tstirrat]

I'm still working on this, just really busy at work this week. I'll get a chance over the weekend to finish it.

[tstirrat]

I'm working towards this API, as suggested by @jayphelps :

var app = new EmberApp({
  outputPaths: {
    app: {
      css: {
        'main': '/css/main.css',
        'theme/a': '/css/theme/alpha.css'
      }
    }
  }
});

Output paths/filenames will be respected regardless of whether you are using a preprocessor or not.

[jayphelps]

[rwjblue]

Any updates here?

[tstirrat]

made some progress but havent had time to diagnose some failing tests.

[tstirrat]

It seems there is some trickery required in merging options below the first level:

// defaults in ember-app.js
{
    es3Safe: true,
    wrapInEval: !isProduction,
    outputPaths: {
      app: {
        css: {
          'app': '/assets/' + this.name + '.css'
        },
        js: '/assets/' + this.name + '.js' // <--- note this
      },
      vendor: {
        css: '/assets/vendor.css',
        js: '/assets/vendor.js'
      },
      testSupport: {
        css: '/assets/test-support.css',
        js: '/assets/test-support.js'
      }
    },
    // snip
}

// user's Brocfile.js
var app  = new EmberApp({
  outputPaths: {
    app: {
      css: {
        'main': '/css/main.css',
        'theme/a': '/css/theme/a.css'
      }
      // note: no 'js' key here
    }
  }
});

// merged options:

{
  outputPaths: {
    app: { 
      css: {
        'main': '/css/main.css',
        'theme/a': '/css/theme/a.css'
      }
    },
    vendor: {
      css: '/assets/vendor.css',
      js: '/assets/vendor.js'
    },
    testSupport: {
      css: '/assets/test-support.css',
      js: '/assets/test-support.js'
    }
  }
}

In this situation, the user would not expect to have to supply the outputPaths.app.js path unless they were changing it.

@rwjblue Given that a deep merge will also give undesired results (it will merge the default outputPaths.css.app key back in), we might need a custom merging method. Would you be ok with that?

I'd prefer not to change the options merging stuff, or start a new pattern, without consulting you.

[tstirrat]

Once we have a correct options merging, this is ready to go from my end

[tstirrat]

@rwjblue I've fixed the options merging issue - with failing test - and rebased. I think this is ready to go.

[stefanpenner]

sorry for letting this linger, it unfortunately no longer merges cleanly

[tstirrat]

@stefanpenner I will rebase

[stefanpenner]

@tstirrat :)

[stefanpenner]

this appears to not be a breaking change can you confirm?

[tstirrat]

I wrote tests to keep backward compatibility:

• without any preprocessors, all CSS files in app/styles/ are copied to the /assets path. (I believe this changed in 0.1.1, but was reverted in 0.1.2?)
• app.css is renamed to <app-name>.css be default (unless you configure outputPaths)
• with a preprocessor and no custom outputPaths, only the app.scss file is passed through (it is also renamed to <app-name>.css)

Correct me if i am wrong, but this should maintain backwards compat.

[stefanpenner]

@tstirrat yes it should, thanks :)

[tstirrat]

I'm not sure how it will interplay with addon styles. I might need some time to digest those changes.

[stefanpenner]

@rwjblue could use your eye re: add ons here.

[tstirrat]

let me rebase, in doing that, i will probably have to make the appropriate changes to get the addon tests passing.

[tstirrat]

I've rebased, the tests pass in isolation .. but I get intermittent errors on seemingly random tests in brocfile-smoke-test-slow.js when running all the tests together..

version: 0.1.2-multiple-css-files-ada95fb9e3


Missing npm packages:
Package: ember-random-addon
  * Specified: latest
  * Installed: (not installed)

Run `npm install` to install missing dependencies.

This is an addon used in a totally different set of tests. which makes me think the default package.json is not being copied correctly in the fixtures.

[tstirrat]

... perhaps it's an OSX thing.

passing fine on travis: https://travis-ci.org/stefanpenner/ember-cli/builds/40491315

@stefanpenner @rwjblue please let me know if there's anything else I can do here.