Only inline @import files if none will be skipped

[ryanfitzer]

The flattenCss function in optimize.js will skip imports that contain media queries with expressions:

@import url('somefile.css') screen and (min-width:768px)

This makes sense. The problem is that the function will still inline other imports that occur before ones that are skipped. These skipped @import rules become invalid and are ignored by browsers.

The W3C spec for CSS 2.1 states in section 4.1.5 At-rules:

CSS 2.1 user agents must ignore any @import rule that occurs inside a block or after any non-ignored statement other than an @charset or an @import rule.

flattenCss should first check if the file contains any imports that would be skipped before inlining imports.

[jrburke]

Makes sense, putting this in the 2.0 bucket as that is the current dev branch and likely next release.

[ryanfitzer]

Cool. I worked on a solution where all @import rules are first checked for expressions. It's about 10 extra lines in the function I can submit a pull request if you'd like.

Ryan

On May 12, 2012, at 10:50 PM, James Burkereply@reply.github.com wrote:

Makes sense, putting this in the 2.0 bucket as that is the current dev branch and likely next release.

---

Reply to this email directly or view it on GitHub: https://github.com/jrburke/r.js/issues/167#issuecomment-5674204

[jrburke]

I'm thinking of changing the flattenCss so that it returns an object structure, that has two properties, whether an import was skipped, and any text that should be used for the inlining. Or something like that -- but basically, just do the file scanning/traversal once, and have a way to signal if the traversal and flattening should be discarded at the top level.

If you patch does something like that, yes please submit it.

[ryanfitzer]

Agreed, the file object makes sense.

For my current project, I've written a method to do targeted builds. It's a responsive design site with a lot of css files and a lot of @import rules with media queries. The method creates a files object and each file in the object looks like the following:

'/absolute/path/to/file.css': {
    hasMediaExprs: true,
    imports: {
        '/absolute/path/to/import-file.css': {
            rule: '@import url( \'404/600.css\' ) screen and ( min-width:600px );',
            mediaTypes: 'screen',
            mediaExprs: '( min-width:600px )'
        }
    }
}

The files object is created by looping through each file and getting its contents and then looping through each @import match in the contents to create the imports member.

Once the files object is built, I then loop through each file object and check the hasMediaExprs (for the current file and its imports). If everything is clear down the chain, I run requirejs.optimize() on the file. Any file that has media query expressions down the chain is ignored.

Each imports member has its rule broken out into mediaTypes and mediaExprs because I can see that a user may want the option to inline the import into an @media {} block. Having these properties will allow the block to be easily built.

If this object structure makes sense to you, I can integrate this into flattenCss so you could give it a spin. It's much more code than my original solution (which couldn't check a file imports recursively).

Thoughts?

[jrburke]

@ryanfitzer: I'm still new to the ins and outs of media queries, so your comment is helpful. I was thinking of just not doing any inlining if there was an @import with a media query. But I suppose it is possible to inline them if they are wrapped in an @media, is that right?

Maybe the inlining code could just always @media wrap the imported code? Do you think that would be too much to inline?

I'm trying to find the right balance of configurability vs. just doing the most likely best optimization. Also if there is just one @import that cannot be inlined, it seems a waste to not be able to inline anything.

Maybe I just need to "hoist" the @imports that cannot be inlined to the top of the file? I feel like that would break the CSS cascade, or rather change it vs. what the developer saw during development.

[ryanfitzer]

But I suppose it is possible to inline them if they are wrapped in an @media, is that right?

Correct.

Maybe the inlining code could just always @media wrap the imported code? Do you think that would be too much to inline?

The problem there is that you'd lose the ability to structure your files and their dependencies for best caching. Caching @import files has good support these days. We were taking the "inline everything" route originally (via LESS), but quickly realized that there was a lot of global css that could take advantage of browser caching.

With that said, inlining any @import would necessitate inlining all imports that come after it the order. Right now my solution simply ignores the whole file if media queries are found.

Maybe I just need to "hoist" the @imports that cannot be inlined to the top of the file? I feel like that would break the CSS cascade, or rather change it vs. what the developer saw during development.

Yeah, the hoisting would greatly conflict with any css that was media query inlined. The cascade become doubly important in this case.

I'm trying to find the right balance of configurability vs. just doing the most likely best optimization.

I agree. I'm all for minimal configuration. I previously read that you were thinking about breaking the css optimizer out into it's own tool. Having it be extendable to provide more options makes sense to me if that's still the case.

[jrburke]

I committed a fix for this in the near term, just not inline any @imports if they are found, see the commit above. I would like to leave this ticket open for a bit though to see how we could allow for some kind of config that would allow streamlined builds of targeted media queries -- in that case it would just remove the media queries that were not part of the target. Or maybe that does not make sense, and is trying to get too optimized. If you think so, feel free to close the ticket for now, but otherwise, let's keep talking it through.

I have decided for now to not break out the optimizer, see #102 for info, but that was more out of a "not sure how to do it vs. time needed/payoff to do so" decision, and I want to wrap up the code for a 2.0 release candidate.

Taking this out of the 2.0 milestone bucket for now, but not closing it yet.

[ryanfitzer]

Makes sense. I agree that more thought needs to be put into a config strategy. Here's where I'm at so far. I've written a targeted optimization routine that takes a first pass through each css file and turns:

@import url('some/file.css') screen and (min-width:600px);

into:

@media screen and (min-width:600px) {
@import 'some/file.css';
}

This file is then handed off to requirejs.optimize for inlining (via 1.0.8) as normal. I end up with a properly minified and concatenated file.

The sticky points reside in being able to have a strategy that allows one to configure which files should be inlined and which to preserve as an @import. This would be a non-issue if @import rules were allowed to fully participate in the cascade part of CSS instead of only being allowed to exist before all other styles (as previously discussed).

I'm mitigating this issue by how I organize my @import rule dependency chain. Only the top-level css file can declare a dependency that can be ignored. All of it's dependancies can import anything they want, as long as those imports are always inlined. While I have yet to write any error handling for this strategy, I intend to as it's easy to do via the files object I wrote about earlier.

Supporting a convention like this would allow for a simple config object. One that mimics the modules config would make for consistency:

cssModules: [
  {
      name: 'some/file.css',
      ignore: [
          'some/other/file.css',
          'and/another/file.css'
      ]
  }
]

The name property would act as the id that mirrors its index in the files object. The files object uses absoute paths, so name would have to be resolved in a reliable manner to create an absolute path. Adding a cssBaseUrl perhaps?

The ignore prop would default to a boolean with false as its value and inherit whatever routine is determined by value of optimizeCss. If ignore is true, the file is totally skipped. It might also make sense to allow any value that the cssImportIgnore property allows so to enable overwriting the global default on an individual basis.

This conventions feels somewhat analogous to how circular js dependencies are handled. Except with css, all a files dependancies would simply be ignored.

My build file is a little too tightly coupled to my project at the moment. I'm working to change that and will post an example to play around with soon.

Thoughts?

[jrburke]

I have been meaning to comment on this more, but I'm weak on media query rules.

I like the idea of doing an initial transform that converts the @import to a braced media thing with the @import inside. The cssModules approach that uses full paths seems to make sense too. Maybe cssLayers to indicate these are not module IDs but paths. A cssBaseUrl seems to make sense in that case.

I'm also wondering if there is way to use something like matchMedia to indicate what media queries should be kept in the build. But I'm just throwing out buzzwords, not sure how it all wires together.

[ryanfitzer]

I'll get a demo project together soon so you can give some feedback on what I've implemented.

Regarding using matchMedia, that is a browser-only API, as far as I know. It needs the window object to test queries against. It's useful for branching js behavior. Not sure it would help for building CSS.

On Jul 7, 2012, at 9:50 PM, James Burkereply@reply.github.com wrote:

I have been meaning to comment on this more, but I'm weak on media query rules.

I like the idea of doing an initial transform that converts the @import to a braced media thing with the @import inside. The cssModules approach that uses full paths seems to make sense too. Maybe cssLayers to indicate these are not module IDs but paths. A cssBaseUrl seems to make sense in that case.

I'm also wondering if there is way to use something like matchMedia to indicate what media queries should be kept in the build. But I'm just throwing out buzzwords, not sure how it all wires together.

---

Reply to this email directly or view it on GitHub: https://github.com/jrburke/r.js/issues/167#issuecomment-6828900

[jrburke]

On the matchMedia, I was thinking more about using the same kind of way to specify what media queries to keep in a build, just as a way to reuse common apis, even though the implementation may be different. But it still might not make sense.

[ryanfitzer]

For css optimization config options, I propose storing them in a css object. When the css option is present, it supersede's optimizeCss and cssImportIgnore. This way both can be supported during transition (if they get deprecated).

Here are the basic options I feel are needed:

    css: {

        /*
            All css files are located relative to this path.
            If not explicitly set, then all files are loaded 
            relative to the directory that holds the build file.
            If `appDir` is set, then `css.baseUrl` should be
            specified as relative to the `appDir`.
        */
        baseUrl: 'css/',

        /*
            By default, each file is optimized with the following options:
            - @import inlining
            - line return removal
            - comment removal
        */
        optimize: {
            imports: false // Preserves @import rules
            lines: false, // Preserves line returns
            comments: false // Preserves comments
        },

        /*
            Array of of files to not optimize.
            The file paths are resolved relative to `css.baseUrl`.
        */
        ignore: [
            'path/to/file.css'
        ]
    }

I've left out equivalent support for cssImportIgnore because of the following caveats:

1. the @import rule to ignore must be positioned before all other @import rules:

   CSS 2.1 user agents must ignore any '@import' rule that occurs inside a block or after any non-ignored statement other than an @charset or an @import rule.

2. any file that directly or indirectly imports the ignored file will be ignored to prevent the first caveat

Another reason is due to browser performance. Files loaded via @import rules are blocked from loading until their parent file is fully loaded. I was originally for it, but now realize load blocking definitely outweighs the caching benefits.

_On Tue, Sep 4, 2012 at 9:02 PM, James Burke wrote_:
Do you know of any good CSS parsers, something that creates an AST or token stream? I wonder if it is worth using something like that, if it helps make for instance the config options easier to apply since we can parse the CSS better.

Maybe it is not needed, CSS is more uniform in shape than say JS though. Just thinking out loud.

Since we only need to find the @import strings, I think a CSS parser would be overkill. My regex still needs a little more work to properly tokenize @import rules that contain multiple media queries, but it's very solid otherwise.

[jrburke]

I wanted to get this in for 2.1, but my time window to work on 2.1 is closing, and there is enough in 2.1 as far as fixes that I want to get it out in this time window. I also wonder if it is better to prototype this as a separate tool. Sort of related to #102, in that it would be good to just use as a standalone tool. In any case, it unfortunately will not make the 2.1 window, and given my expectations of the day job over the next month, it is unlikely I'll have bandwidth to contribute much to this.

[ryanfitzer]

I came to the same conclusion regarding a standalone tool. I've made some progress on it but my day job workload has been more than I expected expected.

Pushing to 2.2 is no problem. I'll get a repo up for this soon.

On Oct 1, 2012, at 10:07 PM, James Burke notifications@github.com wrote:

I wanted to get this in for 2.1, but my time window to work on 2.1 is closing, and there is enough in 2.1 as far as fixes that I want to get it out in this time window. I also wonder if it is better to prototype this as a separate tool. Sort of related to #102, in that it would be good to just use as a standalone tool. In any case, it unfortunately will not make the 2.1 window, and given my expectations of the day job over the next month, it is unlikely I'll have bandwidth to contribute much to this.

— Reply to this email directly or view it on GitHub.

[ryanfitzer]

He's what I have so far: CSSCat. There's still a lot of work to do, but I think it makes for a good start, as it covers all the use cases that r.js currently handles, except:

• minification is all-or-nothing
• no equivalent option for cssIn and out
• Node-only, no Rhino equivalent (need to look into this more)

Some next steps needed are:

• a lot of testing
• making easily consumable by r.js as discussed in #102

[ryanfitzer]

Quick question: in #102 you talked about env dependent files (Rhino/Node), from I can tell it's just optimize.js and file.js that branch due to env, correct?

I would like to find a compatible way for r.js to use csscat and my thought was for some option to pass the env-dependent modules to csscat. This way r.js could configure csscat before using it.

[jrburke]

I have not looked in depth at csscat yet, but thanks for diving more into this! Yes, I think the big thing, maybe from csscat's perspective, is being able to pass in a function that could be called to do a file read, maybe that is enough, if the csscat method returns a string. Although, maybe there is issues with relative path resolution. Might be worth a shot though. The r.js optimizer could take the return string value maybe and save it out to the right place.

I'm just going off pure imagination though, you might have some other API in mind which is fine too.

[ryanfitzer]

After more thinking around compatibility, here's where I'm at:

1. I've added a files option that takes an array of file paths.

   The paths are currently expected to be relative to the dir option, but I could also add a check for a leading slash and if found, treat them as absolute. This would allow r.js to use it's own resolution procedure.

   Resolving the paths found in the @import statements against their ancestor's path is done with Node's path module, so I need to take a look at how r.js handles this based on env.

2. For optimization, my idea is to add a callback option.

   The function would be passed the file's content (after all dependencies have been imported) for alternate optimization. YUI's cssmin is what I'm currently using and it doesn't provide all the options r.js does (or they're simply not documented).

3. Regarding file I/O, exposing a way to provide csscat an alternate readFile and writeFile shouldn't be a problem.
4. Lastly, making it easy for r.js to consume csscat as a dependency involves using amdefine, correct? I need to look into this module more. Hopefully I can make a simple adapter module.

Thanks for the reply. I'll update the thread when I make some progress on these.

[jrburke]

I'm way behind on things, but as far as consuming in r.js, you do not need to use amdefine. I normally use the commonjs.convert method to import code that is just node based. If it has deep node dependencies, that may be a problem. But end result is that you do not need to use amdefine unless you want to code the modules in amd format and want it to run in the browser without a commonjs.convert() call first.

[ryanfitzer]

I've done a rough integration of csscat and it was surprising simple!

Basically, I'm running csscat just after the fileList variable is defined. All css @import dependencies are inlined, leaving r.js the room to honor its optimizeCss option.

I'll need to augment this to support the case where cssIn is used, since in that scenario, the css method is never called (the cssFile method is called directly).

There is one sticking point: support for the cssImportIgnore property. This is something csscat does not support. The reason is that it can have a multiplied effect that the user may not expect.

For example, if an @import is ignored anywhere in a file, it requires all files in the dependency chain to be skipped. This is due to @media blocks not supporting nested @import statements.

Here's an example scenario:

/* a.css */
@import url( 'b.css' ) screen and ( min-width: 200px );

/* b.css */
@import url( 'c.css' );

/* c.css */
#some-id { display:block; }

So if the r.js config contained cssImportIgnore: 'c.css', it would mean a.css would also have to be skipped to avoid the following invalid css:

/* a.css */
@media screen and ( min-width: 200px ) {
    @import url( 'c.css' );
}

My initial idea is that csscat would only be run if cssImportIgnore is undefined. This would mean that files that contained imports with media query expressions would be skipped (how r.js currently handles this).

The other option is to deprecate the cssImportIgnore option, as it still has the unintended ability to create invalid css by ignoring any but the first @import statement in a file (the browser ignores @import statements if they're preceded by anything other than specific @ statements).

Any thoughts?

My next steps are to expose the file I/O methods so Rhino is supported and handle the cssIn scenario.

[ryanfitzer]

I ran into a scenario where I had to implement the exact behavior that cssImportIgnore supports (absolute paths and urls). I've implemented ignoring the whole dependency chain if valid css cannot be produced.

I'd caught this when parsing image assets on imported files, but had forgotten to check for the same in @import statements.

So it looks like you can disregard both options I proposed above. I'll add an ignore option to CSSCat to handle the cssImportIgnore property.