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.

[ben-eb]

Sure. Have you thought about function resolution? Given two processors, one that converts add(a, b) to calc(1 + 1) and one that reduces calc expressions;

Input:

calc(add(1,1) + add(1,1))

Output:

4

[lexich]

@ben-eb Yeap. in new syntax solution is like this

createStream([{
   decl: {
      value: (val)=> !!val && val.indexOf("add(") >= 0;
      enter:(decl)=> decl.value = addProcessor(decl.value);
   }
}, {
  decl: {
     value: (val)=> !!val && val.indexOf("calc(") >= 0;
      enter: (decl)=> decl.value = calcProcessor(decl.value);
  }
}]);

[davidtheclark]

In my opinion, this query API seems unnecessary. It makes for a larger API, more documentation, more maintenance — for what? As far as I can tell, it does nothing that a user cannot accomplish with an if-statement and an early return. Why not just suggest that users do that — use JS itself — instead of trying to build additional features that are such light wrappers over simple JS?

@ai Many stylelint rules must know a node's parent, the previous node, or the next node. I think much of this could be done with an event-based API, right? When a new node is created and an event emitted, the parent should be known, I believe; so should the previous node. The only trick is the next node. Could this be an async function — node.next() returns a Promise?

[lexich]

@davidtheclark event api is over existing ast. All each functions have complexity O(n), with stream we reduce complexity. And all methods of old API are available. But main question what performance benefits we'll get with this way, but I think that there is a good chance to speed up postcss plugin infrastructure.

[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
    }
  }
}