Set up CI test service

[vladshcherbin]

I guess, this package is used by almost all users, who want sass/scss support in meteor. There is a recent discussion about adding sass/scss into core.

@stubailo asked, if someone would like to become the primary maintainer. Maybe you are interested? It would be nice if we would have official sass/scss package and mention it in blog posts, etc.

[stubailo]

In particular, I'm suggesting that we can figure out how to set up a CI test service that can check if this package works with every new Meteor version. Once we have a mechanism for knowing if it works, then we should make sure it's easily discoverable to all Meteor users as the thing that works.

[fourseven]

I've wondered about pulling out parts of telescope to use for testing, since it's a pretty full-fledged app, but yes the current test state could use some TLC. It's unfortunate that we essentially have to test node-sass as well.

[sebakerckhof]

I agree, but once the importer functionality of node-sass stabilizes this shouldn't be something to worry about too much, their api isn't likely to change a lot.

Testing with a real app sounds good, but I've been surprised at the amount of use cases I hadn't thought of and I guess telescope doesn't cover all of them either. Sass-style (indented) syntax is one of them.

[fourseven]

It was easier when node-sass only did scss (hah)!

[vladshcherbin]

I do think that almost everyone is using scss now. I also think, that we can have similar tests as less/stylus packages. Basically, we need to test if the class is applied to an element and if the imports work as expected - this will be enough.

[stubailo]

I think having complete test coverage will be the number 1 thing that will make it easy to make this package more reliable across future Meteor releases. In some sense, that's the only real benefit of having a "core" package - that it's QAd extensively for every new release.

It doesn't have to cover all of the node-sass functionality, but it should cover all of the relevant edge cases - cross-package imports, sass/scss, empty/malformed files, and whatever else makes sense.

[sebakerckhof]

Okay, I'll start by porting the less tests to scss/sass and maybe expanding them a bit.

[sebakerckhof]

@stubailo I've added some tests yesterday. For .scss and .sass, but .sass is currently disabled until the bug in node-sass is fixed.
What would be the next steps for the CI service and does it still make sense now that MDG is probably going for a major overhaul of the build system?

[stubailo]

Yeah it makes sense since in the short term people will still want to use your stuff!

So let me know if you have success running your tests against the devel branch of meteor/meteor?

[stubailo]

@sebakerckhof any updates? I'm working on an outline for the build tool section of the Meteor guide, and I'd like to include SCSS/SASS there: meteor/guide#41

Let me know if I can help directly - what issues are you running into, what have you tried so far?

[sebakerckhof]

I forgot about this. My tests run against meteor-tool 1.1.9 (except the tests for '.sass'-files due to a bug in node-sass). I just tried to test against devel branch, but got:

=> Started proxy.
=> Started MongoDB.
=> Linted your app. No linting errors.
=> Exited with code: 8
W20151019-19:49:24.503(2)? (STDERR)
W20151019-19:49:24.504(2)? (STDERR) Z:\programdata\github\meteor\dev_bundle\server-lib\node_modules\fibers\future.js:245
W20151019-19:49:24.504(2)? (STDERR)                                             throw(ex);
W20151019-19:49:24.504(2)? (STDERR)                                                   ^
W20151019-19:49:24.504(2)? (STDERR) Error: The operating system cannot run %1.
W20151019-19:49:24.504(2)? (STDERR)
W20151019-19:49:24.504(2)? (STDERR) Z:\programdata\github\meteor\.meteor\packages\npm-node-aes-gcm\0.1.3_6\npm\node_modules\node-aes-gcm\build\Release\node_aes_gcm.node
W20151019-19:49:24.504(2)? (STDERR)     at Module.load (module.js:356:32)
W20151019-19:49:24.504(2)? (STDERR)     at Function.Module._load (module.js:312:12)
...

Also, I would appreciate your opinion on some topics that pop up here a lot.
In the Meteor 1.1 builds, this package also included or allowed stuff like:
-Importing from the .meteor folder (often used to import bower files using mquandalle:bower package)
-Auto-prefixer
-It could generate an index file that imports all non-partial files, so people only have to state their variable imports once.

These were removed when I rewrote this plugin to the new multi caching compiler and support for cross-package imports etc.
I'd rather not have these back, since they don't fit well in how the caching compiler and meteors build system works (e.g. the index files) and/or are not a part of Sass (e.g. autoprefixer).

However, since people have gotten used to this, there are a lot of complaints about broken apps and the loss of functionality. And seeing David Glasser's answer in this issue: meteor/meteor#5219 , it seems like there won't be a solution anytime soon for things like autoprefixer.

Do you think we should try to put them back into this package?

[stubailo]

I'm not exactly sure about integration with Bower. That sounds like a hard problem, I'll ponder about it a little.

Auto-prefixer, in the current world, would probably be built into the CSS minifier or compiler. So either it needs to be in the SCSS package, or in a new minifier package.

It could generate an index file that imports all non-partial files

Can you tell me more about this?

Also, can you tell me how to run the tests for this package so I can try running against devel on my machine?

[sebakerckhof]

I'm not exactly sure about integration with Bower. That sounds like a hard problem, I'll ponder about it a little.

This does become a lot easier if you put all of your code in package(s)

It could generate an index file that imports all non-partial files

Can you tell me more about this?

See: https://github.com/fourseven/meteor-scss/tree/v3.1.1 under 'Controlling load order [since 2.0.0-beta_3]'.
So this is basically for 2 purposes:

• control load order (again: a lot easier if you put your code in packages)
• Not having to specify an @import "variablesAndMixins" in each scss file.
  However, by doing this you lose a lot of the advantages of the caching compiler, since the caching only works for root files, every file change will trigger a complete rebuild with this method.

IMO, if you really want to avoid the imports, you can maintain an index file yourself. But turns out developers tend to be lazy.

Also, can you tell me how to run the tests for this package so I can try running against devel on my machine?

I only added package tests, similar to the ones of the less package, so you can just clone the master branch and run meteor test-packages

[stubailo]

Great, I'll try running the tests.

since the caching only works for root files, every file change will trigger a complete rebuild with this method.

Can you tell me more about this?

[sebakerckhof]

That's just how meteors caching compiler works: https://github.com/meteor/meteor/blob/devel/packages/caching-compiler/multi-file-caching-compiler.js#L100

Similar to less and stylus the meteor build system will:

1. Watch any files that change
2. For root files (= not imports) it will call the compiler (build plugin)
3. The compiler will compile the file and the whole import tree of that file
4. The build plugin has to tell meteor which files are on the import tree, so meteor knows when to recompile a file ( https://github.com/meteor/meteor/blob/devel/packages/caching-compiler/multi-file-caching-compiler.js#L132 )

However, if you only have one top-level file importing all other files, then every change will trigger a complete recompile of all of your scss/less/stylus/...

[stubailo]

@sebakerckhof I think you're right. Do other build systems somehow enable you to recompile only part of your SCSS dependency tree in that case?

[sebakerckhof]

Nope, since the sass file can only be compiled as a whole.
Say you've got 3 files:
index.scss with contents:

@import  "_variables.scss"
@import "_otherfile.scss"

_variables.scss:

$mainColor: blue;

and _otherfile.scss

.class{
    color:$mainColor;
}

Individually compiled there wouldn't be much to cache for _variables.scss. And _otherfile.scss would need to be updated when _variables.scss changes, but you wouldn't know this.

With some advanced analysis you could maybe avoid compiling some parts, but the analysis would probably cost more time than the compilation.

Therefore, in order to improve build time, I recommend to have not too deep import trees and instead have a lot of smaller root files. Then load order becomes an issue, therefore I recommend to put everything in packages. But people have been doing it otherwise and want to continue doing it otherwise...

[stubailo]

OK so I think we've determined that having one index file might not be the best idea for performance, but is probably OK if you're lazy?

[sebakerckhof]

... if that index file is automatically generated. Which isn't done by the current version of the build plugin.

Another problem with the automatic generation of index files is that if you move or delete a file, you still need to update the index file yourself since the meteor build system will not tell the build plugin about removed files. We only know about changed or new files.

Anyway, I think this plugin used to do some stuff it shouldn't do in the past. But I don't know if I should break all these apps people have and force them into another way of working (while they're all raging: "WHY CANT METEOR DO WHAT I COULD DO WITH GULP"), or just continue putting stuff in this package that doesn't work well together with the meteor build system.

[stubailo]

@sebakerckhof yeah... I would say that auto-importing is not so important. Things you actually can't do like autoprefixer would strike me as a higher priority.

Honestly, maybe we should document how to set up a gulp script if that's what people really want. It's not terribly hard. You'd probably just run it at the same time as the Meteor tool, and put the files in some dotted directory.

[sebakerckhof]

Seems like a good idea. Maybe for autoprefixing we can make an exception since it is so popular, but we can't support every exotic build pipeline people have in mind.
Would it still be possible to make sourcemaps work this way?

[stubailo]

Hard to say - I've heard source maps are one of those things that are super hard to get right.

[vladshcherbin]

Gulp script as the answer? Really? You not kidding me? Scan all the packages for .scss files?

F. the lazy people, force them to create an index file (index, styles, whatever they call it) and import there underscored files in the order they need. It is the best and the common practice.

There is no need to cache some parts, the libsass compiler works blazingly fast and when we import the underscored _file.scss files, we expect them to concat and not to be handled on their own. Don't reinvent the wheel.

The autoprefixer must be a separate package, it has nothing to do with scss. It should grab any css (after compiling from less, sass, stylus, etc) and add vendor prefixes to it, no manner how it was compiled to css.

The main reasons why people use gulp, compass, etc is that we are expecting this to work:

• compile scss files, not the underscored ones [I guess, works now]
• cross-imports [I guess, works now]
• relative paths imports [works in the latest beta?]
• autoprefixer [we don't have it] (not the problem of this package)
• we need it to simply work and don't break on every update

[sebakerckhof]

@vladshcherbin You can still generate index files manually.

Autoprefixer should indeed be a separate package, but the current meteor build system will not easily allow this. My current idea is to create a separate branch for scss+autoprefixer, since autoprefixing is so popular, but I can't do this for every combination of css processors.

Relative path imports indeed work in the latest beta.

I also have a branch that scans for file paths beginning with .meteor and allows to import these file while giving a deprecation notice. These files are not watched by Meteor and will therefore not trigger updates. However, since this is mainly used to import files from bower, which don't change a lot, I guess that is ok for now.

[vladshcherbin]

@sebakerckhof yes, so basically the package does the job and we can compile scss with all features.

The last thing we need is the autoprefixer as a separate package. @stubailo , my hero, is there a way to add a step with css post processing or update the build system to support adding this step?

[stubailo]

@vladshcherbin right now, it's not feasible to add another stage to the build system, which is why I was looking for alternative approaches. Another thing we could do is package autoprefixer as a minifier - so you could replace the standard CSS minifier with one that does autoprefixing + minification.

How many more CSS processors are there that really matter other than autoprefixer? Maybe it's even something we should enable by default?

[vladshcherbin]

@stubailo yes, having two packages for minifying and autoprefixer + minifying may be an option. It also can be a default as I don't know, how it can harm anyone. Maybe mobile dev just don't need it, but 1-2kb is not a big deal I guess.

Actually, the main thing is the postcss and there are lots of plugins for it. The autoprefixer is just the most needed and used one (even has more stars than the postcss itself, haha).

[stubailo]

@vladshcherbin I think maybe what we need in the short term (before we overhaul the Meteor build system to be more inherently extensible) would be some sort of postcss + minifier package, where you configure it with a JSON file in your app - select which postcss plugins you want, pass them options, etc. I feel like this is getting really really off topic in this thread, let's move it somewhere else like the forum?

[vladshcherbin]

@stubailo yes, this may be a great and flexible workaround. So, we will have the working scss package and many post processing css packages.

Can you open the forum topic to get another opinions on that? I kinda don't know how to name it and what to write inside. Something like [Proposal] Adding postcss into the build-system?

[stubailo]

Haha I was hoping someone else could do it since I don't have the time to think about it at the moment.

[vladshcherbin]

@stubailo done ;)

[sebakerckhof]

Anyway, to get this back on track: On linux I'm able to run the tests against the devel branch of Meteor (and they pass)

[stubailo]

@sebakerckhof are you able to set it up on Travis CI?

[sebakerckhof]

I'll have a look, but no experience with Travis. Here everything is stash(unfortunately)+jenkins.

[sebakerckhof]

I've set it up on my branch, but the tests seem to fail in travis. I tested them once more locally and found out they work in Chrome and IE, but fail in firefox :s, maybe it's the same reason why they fail in phantomjs on Travis. Are there any known issues with some tinytest stuff (in particular test-helpers and getStyleProperty) and certain engines?

[ghybs]

Hi,

Already submitted PR #243 and found this open issue…

Looks like the root cause for Travis test failure is exactly what @sebakerckhof guessed: getStyleProperty is able to correctly read synthetic styles like border-style in some browsers / engines (like Chrome), but not in others (like FF), in particular in PhantomJS, which is the one used in the Travis setup.

Simply reading one of the underlying individual styles (e.g. border-top-style) solves the issue.