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

Facilitate autoprefixer needs #20

Closed
necolas opened this Issue Jun 4, 2014 · 14 comments

Comments

Projects
None yet
5 participants
@necolas
Contributor

necolas commented Jun 4, 2014

this issue mostly for discussion, but as long as we share ASTs we can have better interoperability. I totally get the need for node-level utilities to inject declarations etc, but I think we could have a suite of tools (or nodes "classes") on top of the vanilla AST

the most "elegant" thing to do would just be piping streams of things that manipulate the CSS, but since that's slow and causes needless re-parsing it would be great if we could either a) make this parser facilitate autoprefixer's needs b) spec the AST to maintain compatibility


@ai said:

I think we should:

  1. Try to parse custom at-rules. By spec, unknown at-rule can contain any content. By all current at-rules (and custom at-rules from frameworks) contains only rules or declarations. The best solution is:

    • Try to detect content type (rules or declarations) by names.
    • Try to parse rules or declarations inside.
    • If we can’t parse rules content, we should store just as string.

    But it is ideal way (in PostCSS I had no time to realize it in PostCSS) :).

    I had a lot of issues from new at-rules or special JS libraries. Also, it is easy to configure some Rework plugins by custom at-rules. Like:

    @sprites {
      padding: 10px
    }
  2. Saves all outside spaces. Of cource, we should do it by special option.

    This is important for text editor and keep style from uncompressing (some users use Autoprefixer after Sass with :compressed output, some users use win text editors).

  3. Integration tests, to understand that we parse most of production CSS. For example, previous version of Rework had issue with parse even Bootstrap. I test GitHub, Twitter, Bootstrap (a lot of hacks for old IE) and Habrahabr (Russian IT Reddit with very bad CSS files as example, how developers can write :) ).

    Bad written CSS is importnat for me, because a lot of Rails project use some 3th part CSS from libraries and had problem with Autoprefixer, because they can’t fix libraries (like Bootstrap).

  4. Saves comments inside values. There is some difficult situation with comments inside declaration’s values and at-rule parameters. We should clean values from comments for Rework plugin developers, but we need to keap origin raw dat with comments to stringify (PostCSS has special hack with raw properties).

But this PostCSS feature is only for integration test. I just check, that PostCSS’s parse/stringify doesn’t change any bite of production styles.

And I will be glad to remove postcss/parse.coffee and use PostCSS only as object high-level API under the css-parse AST.

BTW, I didn’t think that fragmentation is really bad. In real world we support diversity for languages (there is Red List for language) and biologically species (IUCN Red List). Diversity is important to evolution (hieroglyphs is very unusable but we hasn’t “cool” tattoo or Matrix digital rain if China moved to phonetic alphabet). But it is only philosophical note, I think most of Autoprefixer users will be glad to come back to Rework with PostCSS features :).

(Moved from https://github.com/reworkcss/css-parse/issues/68)

@necolas necolas added the enhancement label Jun 4, 2014

@tj

This comment has been minimized.

Member

tj commented Jun 5, 2014

assuming it's fast enough we could also just go with byte streams for the unified level of api, that loses some flexibility though, and would drop perf by quite a bit, but it would be sort of cool to just do:

$ cat in.css | rework-vars | rework-extend | autoprefixer > out.css
@conradz

This comment has been minimized.

Contributor

conradz commented Jun 5, 2014

@visionmedia now that css applies original source maps, this sort of thing should be pretty easy to do while preserving the original source locations.

@lydell

This comment has been minimized.

Contributor

lydell commented Jun 20, 2014

Number 1, Try to parse custom at-rules, is planned for css 3.0 in #16.

Number 3, Integration tests, is something I think we definitely should do.

Number 4, Saves comments inside values, is something I proposed in #24.

Then we have number 2, Saves all outside spaces, left. Is that something that this module should do?

To summarize this issue, it is about two things:

  • The autoprefixer users demand super robust CSS support parsing wise. That’s something that this module should aim for.
  • autoprefixer wants to be able to work as a text editor plugin. I’m not sure that this is a goal of this module, though.
@ai

This comment has been minimized.

ai commented Jun 20, 2014

I think saving outside spaces is important not only for text editors:

  1. It allows to build minifiers and beautifier.
  2. I think is very important to respect input code and changes only what user ask. If I add some small postprocessor, that change only few lines of my CSS, I expect that I will see my origin CSS in server output, not totally different.
  3. If we saves spaces, it will be very easy to check, what postprocessor do. Just compare input and output with diff.
  4. Sometimes users process readable CSS for development. Sometimes they process minified CSS for production. Of course, css has option to change minified/readable output. But the best settings UX is when you have not any settings ;). If we will saves all outside spaces, we will save small size of minified CSS and readability of development CSS.
@tj

This comment has been minimized.

Member

tj commented Jun 20, 2014

maybe we need a cssfmt like gofmt, most css I've seen where people want to retain whitespace, happen to use really weird disgusting whitespace, but as far as this project goes I agree it would be nice to retain

a cssfmt would be nice though!

@necolas

This comment has been minimized.

Contributor

necolas commented Jun 20, 2014

there's csscomb that's probably the closest to something like a cssfmt for now - https://github.com/csscomb/csscomb.js

@ai

This comment has been minimized.

ai commented Jun 21, 2014

BTW, Autoprefixer has option to output prefixes cascade:

a {
  -webkit-transition: 1s;
     -moz-transition: 1s;
          transition: 1s;
  color: black
}

Of cource, it is possible only when we control outside spaces.

@lydell

This comment has been minimized.

Contributor

lydell commented Jun 21, 2014

Actually, saving all whitespace makes a lot of sense now. What about this:

  • css.parse: Save all whitespace in the AST somehow (and all inside comments).
  • css.stringify: Turn the "identity" compiler into a rework plugin (rework-pretty or something).
  • css.stringify: Turn the "compress" compiler into a rework plugin (rework-compress or something).
  • css.stringify: Should just have one compiler that simply uses what is available in the AST, without formatting anything on its own.
  • Create a rework-mimic plugin, which formats the indentation of programmatically added nodes similar to surrounding nodes (so that every plugin doesn't need to think about it).
@tj

This comment has been minimized.

Member

tj commented Jun 21, 2014

that cascading looks really weird to me :D

@necolas

This comment has been minimized.

Contributor

necolas commented Jun 21, 2014

I'm not convinced that there is that much value in exactly preserving whitespace in Rework. This is a build tool, so it should only matter if it affects your ability to perform manipulations on the CSS. For me, it's definitely the least important of these requests.

@tj

This comment has been minimized.

Member

tj commented Jun 22, 2014

Agreed, I couldn't care less personally, just because I would love for people to stop writing retarded CSS to begin with. Retaining it is "ideal" but IMO not worth the baggage either

@lydell

This comment has been minimized.

Contributor

lydell commented Jun 22, 2014

Btw, isn’t that cascade already possible to do (although perhaps a bit hackily), by doing something like declaration.property = " " + declaration.property?

@tj

This comment has been minimized.

Member

tj commented Jun 22, 2014

@lydell yeah you could just indent it sprintf-style

@ai

This comment has been minimized.

ai commented Jun 23, 2014

@lydell if I will add spaces to property it will broke property check in next postprocessors

if ( decl.property == 'content' ) {
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment