Feedbacks

[lexich]

Task description: http://cultofmartians.com/tasks/postcss-events.html

[lexich]

@ai What do you think about this syntax stream-api and early reference implementation?

[ai]

Could you show some examples or links?

[lexich]

implementation postcss-color-function
unit-test

[ai]

query callback and fn?

Hm. Babel use shortcut like:

decl: {
  enter: decl => {
    if (otherCheck(decl)) {
      doSomething(decl)
    }
  }
}

I think this syntax is more compact and still flexible. What do you think?

[lexich]

Yes, current syntax more verbose and of caurse need to improve.
But I think to allow more complicated expression. Maybe more shorten variant?

createStream([{
   // this means than we match expresstion atrule > rule > decl according query composition
   decl: [<query>],
   rule: [<query>],
   atrule: [query],
   enter: node => {

   }
}, {
 ......
}])

But another hand, some shortcurts also good.

[ai]

What type of query you want? Could you show some examples?

[ai]

Or query is just a function?

I think it is still more complicated than 1 function (because 2 functions is more complicated than 1 :D ) and longer.

Why you prefer this syntax with separated query and change functions?

[lexich]

Hmmm.
<query> in my sample can be string, regexp, function, array of string (or/and) regexp. Also may be add shortcut for indexOf.

I thought about your questions and what do you think about improvements in DSL like these.

createStream([{
  decl(node) { 
    // match all decls
  }
}, {
  decl: {
    prop: "width", // or may be function, /width/, ["width", "height"], [/width/, "heigth"] etc.
    value: "100px",
    enter(node) {
      // match all decls with prop "width: 100px"
    }
  }
}, {
  rule: {
    selector: /.test/, // maybe string, function, like example above ,
    decl(node) {
      // match all decl under the rule
    }
  }
}])

I think that this concept like your suggestion and also show my idea of nested queries and of course less complicated

[ai]

Note, that enter is part of enter/leave paradigm.

For example, we have rule a { … }. Some hooks should be executed before processing declarations inside. Other should be processed only when all hooks will finish working inside.

As result, if we have this css: a { color: black }, events will be:

• enter a rule.
• enter color decl.
• leave color decl.
• leave a rule.

[ai]

OK, I am OK with this queries.

What if you need to check other nodes inside with “event listener”? Or we could ignore this case, because we will have event API and old AST API for simple cases (most of custom Stylelint rules could not be written on event API).

@ben-eb @davidtheclark what do you think about this API?

[lexich]

.test { width: 0px; }

createStream([{ 
    decl: { // step1
        prop: "width"
        enter(node) {
            node.value = "100px"
        }
    }
}, {
    decl: { // step2
        prop: "width",
        value: "100px",
        enter(node) {
            node.prop = "height";
        }
    }
}, {
    decl: { // step3
        prop: "width",
        enter(node) {
            // won't perform
        }
    }
}])

Also I wanted to mention that steam listen node modification.
step2 will be execute because step1 fix node value and step 3 won't because step2 changes prop. Also node can be processed or skip by walker only once, independs from it's state.

[ai]

@lexich yeap, changes listening is a great feature. Same with inserting new nodes?

[ai]

@ben-eb right now I am thinking to always keep non-event and event API. Main reason — writing a Stylelint rules should be extremely easy. But all my experience with event parsers (like SAX for XML) told me that event-based API is very complicated. It is much easy to write XML tool on top of DOM, instead of SAX.

In other hand only few PostCSS plugins are really popular. So we could move only slow or popular plugins to event based API (Autoprefixer, CSS Modules, css-loader, cssnano, PreCSS, core Stylelint rules, postcss-inline-svg, postcss-assets, postcss-sprites, RTLCSS).

In this case, I think we could be from doing 100% things in event based API. For example, we could not have API for creating a visitor in visitor (like Babel does, more complicated thing to understand).

[lexich]

@ai yes, to wrap all mutation operation. I have already made this for value property https://github.com/lexich/postcss-stream/blob/master/src/overwrite.ts

[ai]

You making postcss-stream as proof-of-concept for latest PostCSS implementation? Could you rewrite it from TypeScript to JavScript. I like TS, but I don’t use it daily and it is hard to me to read sources :(.

Anyway PostCSS is written on JavaScript, so migration will be more easy.

[ai]

Users want to see properties. So when they will do console.log(decl) they should see { prop: 'color', value: 'black' }. Could you do it with this Object.defineProperty? Also how do you tract Node#nodes changes?

[lexich]

I plan to overwrite prototype for insertBefore/append and other methods, but I haven't implemented this yet. For console.log() maybe it need to overwrite toString.
About typescript, it's very easy to refactoring code and typescript very transparent compiled to js and migration to js it's not a problem.

[ai]

I agree about refactoring. So TS is best for internal projects. But it is hard for other to read this sources. This is why community driven project should be in JS. You could keep use TS, but I just can’t read them with same quality as JS.

I rewrote PostCSS from CoffeeScript to JS only for others ;).

[ai]

console.log doesn’t use toString(). Maybe you should think about some other way to detect changes.

[lexich]

I think that the only alternative way add new mutation api for properties, or underscores for properties.

[ai]

There is a very big problem with _prop instead of prop — most developers don’t read a docs and prefer to understand project API in development (mostly by console.log).

As result, if they will see { _prop: 'color', _value: 'black' }, they will just use node._prop ;).

Note, that PostCSS should works in node.js 4 and main browsers. For example, it will be very hard to use Proxy for observe changes, because this API doesn’t work in IE 11 and could not be polyfilled by Babel.

Especially sad, that Babel visitor API doesn’t need this tricky Proxy things ;).

[lexich]

Maybe dirty checking can fix situation for properties, but it very hard and performance waste way. May be hide props and people can't see props at all, of for example add help property with message { help: 'for trace node print "console.log(node.toString())"' } or some thing like this.

[ai]

We could use Proxy for all browsers and some slow hack for IE.

[lexich]

Not bad idea 👍

[ai]

Also note that Containier#nodes could be changed by direct access:

rule.nodes.splice(1, 1, replacedDecl)

[lexich]

Yes, direct access is headache, otherwise if somebody wanted to crash program, he can find 100500 ways to do this.

[ai]

We could ignore the weirdest way to change nodes (because users will write new code and test it for events API), but we should support common JS API. For example, some cssnext plugins used direct Container#nodes access.

As option, we could use Proxy+slow dirty check on arrays too.

[lexich]

By the way. Streams can work effectively only when they are connected and old (current) plugins don't influence on stream flow, because streams work inside postcss-stream scope (one plugin) and of course we can combine imperative and declarative plugins. Task of mutation is actual only for new Api, current Api will be works without changes.

[ai]

Sure. I think we could execute all event-based plugins in the end after old API plugins.

[ai]

@davidtheclark event API is not stream-based API. We still have full AST in memory. We need event based API to run all plugins in same walk through AST.

[ai]

I like the idea of query because it could reduce calling a callback. For example, if I wrote a plugin for calc() function I don’t want to execute my callback on every function.

[ai]

@lexich seems like you got nice feedback. Next move is to create Proxy/fallback observers

[1j01]

If addProcessor and calcProcessor are just supposed to use postcss-value-parser, then it sounds like this does not solve nested functions. What is this trying to solve?

You need to be able to do color(color(red contrast(50%)) contrast(50%))
which should be equivalent to color(red contrast(50%) contrast(50%)

[ai]

@1j01 we need to steps to fix nested functions problem:

1. Event API
2. Value parser in PostCSS core

This task is about only first step. But It is OK to do first step and then go further.

[lexich]

@ai What do you think about partial replace original api, because seems, that Proxy is very radical way. For example mutation properties like value, prop, selector, etc with simple types will check with dirty checking.
parent - will be available only by function call, it allow to mark this node for dirty checking.
nodes - won't be available at all.

Example

function Wrapper(_node) {
   let node = _node;
   let dirty = false;
   return {
      value: node.value,
      prop: node.prop,
      setNode(_node) {
         dirty = true;
         node = _node;
      }
      isDirty() {
         return dirty || this.value !== node.value || this.prop !== node.prop;
      },
      parent() {
         return Wrapper(node.parent);
      }
      ....
   }
}

[ai]

@lexich I am OK to change AST API (for events plugins only). But how you see to use this API? How I will have access to node’s child?

[ai]

@lexich also this Wrapper is a own copy of Proxy? :D If you need wrapper, maybe you should base it on top of Proxy?

[lexich]

@ai rule api allow describe all hierarchy from top to down.

rule: {
 selector: ".test",
 decl: {
    .....
  }
}

But of course there is way to write accessor for nodes

nodes() {
   return node.map(Wrapper);
}

But in this case mutation of array will be failed and i think that it's not the best way.

[ai]

I like:

nodes() {
   return node.map(Wrapper);
}

Also you need to have nodes= setter to mark container as dirty.

But again, this wrapper case is ideal for Proxy :D.

[ai]

Any updates?

[lexich]

@ai No, holidays :)

[lexich]

@ai I have changed algorithm. According new plan we don't need listen property changes (and now don't need property and dirty checking valuable properties).
All nodes now have 2 additional number properties which indicates id of rules which were applied.
We check nodes until this props won't indication that all rules were applied.
https://github.com/lexich/postcss-stream/tree/traverse

[ai]

@lexich could you show some examples of this 2 additional number properties? Sorry, I can’t read TypeScript sources :(

[lexich]

@ai When solution would be stable, I migrate code to js - I promise :)

At current implementation:

Node {
   ....
   _refEnter: 0,  // id of last visitors which check node on enter 
   _refLeave: 0  // id of last visitors which check node on enter
}

Maybe it's possible to make only one, anyway work in progress and current solution looks better then previous.

[ai]

Looks nice. But how it free us from Proxy? How do you detect changes to execute plugin again on changed nodes?

[lexich]

There is loop https://github.com/lexich/postcss-stream/blob/traverse/lib/src/traverse.js#L37-L40
If some visitor apply its login, isDirty flag is set as true. This means that we need check ast once again and find nodes which weren't checked by visitors.

For example:

[{
  decl: {
     enter(decl) {
       decl.parent.cloneBefore({ selector: ".test1"});  // Here we clone new node and insert before loop
    }
  }
},{
  decl: {
    enter(decl) {
       decl.value = "newvalue"; // this will apply for new node on second loop
    }
  }
}

[lexich]

And i publish es6 compiled code for you. Almost the same as I would write if i used typescript

[ai]

Smart move. It it works only for new nodes. Not for changing exist nodes:

decl.parent.nodes[0].selector = 'a' // it should trigger plugins again

[lexich]

Ok. Workaround: at this moment all visitors applies for each node, but we can apply for each visitor all nodes. And this node will have refs of current visitor. But complexity would be the same. And It helps reduce additional check loops.

[ai]

Sorry, I didn’t get how it will fix problem. Here is example:

Our CSS is:

a {
  width: 100px;
  magic: width;
}

We have 2 plugins: postcss-calc and postcss-magic. They was applied to width and both ignored this declaration. postcss-calc was ignored magic declaration. but postcss-magic was execute callback:

decl.prev().value = 'calc(300px - 100px)'

Now our CSS is:

a {
  width: calc(300px - 100px);
  magic: width;
}

In this case we need to run all plugins on changed to width (to apply calc() plugin).

How do you plan deal with it?

[lexich]

Order of plugins (visitors) is very important. We need to change order. First of all apply postcss-magic and after that postcss-calc. It's normal behaviour.

[ai]

It will not help.

1. postcss-magic was ignore width, postcss-calc was ignore width.
2. postcss-magic changed previous declaration (width), postcss-calc ignored magic.
3. the end

[lexich]

Yes it's current behavior. But new plan.
postcss-magic: 1. Ignore width 2. change declaration.
postcss-calc: 1. convert calc 2. Skip magic
check loop.
End

[ai]

You want to run all AST loop for every plugin? But main point for this Event API is having 1 loop for all plugins. It will increase performance.

Loop-per-plugin is already in PostCSS API and works slow.

[lexich]

No. Complexity will be equivalent. N - nodes M - visitors.

for (n of N)
   for (m of M)
      f(n,m)

is the same

for (m of M)
   for (n of N)
      f(n,m)

In current plugin system works as

for (m of M)
   for (n of N) //eachDecl
      f1(n,m)
   for (n of N) //eachRule
      f2(n,m)
   ...etch

[ai]

@lexich you don’t need to use eachDecl. walkDecls give you same

for (m of M)
   for (n of N)
      f(n,m)

[ai]

I checked latest node and seems like they change console.log behavior. Now it displays getter/setters in correct way. So we could mark nodes as dirty by ovverride all node method and set getters/setters.

[lexich]

@ai hmm. Latest node it's ok, but other nodes and browsers have problem with full support proxy. in current master one test fails on node 4 (polyfill) and pass on node 7. In other hand Proxy is slow and it may downgrade current performace.

Other way without proxy is very like on current plugin system and can be short to this snippet.

css.walkQuery({
   decl: {
      enter: (node)=> node.color = 'red';
   }
});

But in my sense it'll have profit only if you write you plugins very unoptimize way.

As the result we can add new syntax for writing queries, without performance profit (but of course without benchmarks it very hard to say about it), of course if it'll be interesting for project.

[ai]

Let’s just add getter/setter for each Node property