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

Event based API #296

Open
ai opened this Issue Apr 13, 2015 · 44 comments

Comments

Projects
None yet
10 participants
@ai
Member

ai commented Apr 13, 2015

Right now PostCSS behavior depends on plugin order.

For example, if you put postcss-siple-vars before postcss-mixin, it will broke mixins.

@1j01 told about same problem with functions.

And potentially we may have a loop problem. We need to find solution.

@ai ai added the question label Apr 13, 2015

@ai ai referenced this issue Apr 13, 2015

Closed

API for functions #178

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Apr 13, 2015

Member

Brainstorm:

  1. Plugins pack, where order is fixed. Not fix a pack conflicts and loop problem.
  2. Plugins can be reordered by some metadata in plugin description. Like a blacklist. But it requires good feedback from users.
  3. Some event based API, so all plugins will works together. Most complicated and promising way. It fix loop problem and should be faster. But not all plugins can use it (some plugins like Autoprefixer requires look back).
Member

ai commented Apr 13, 2015

Brainstorm:

  1. Plugins pack, where order is fixed. Not fix a pack conflicts and loop problem.
  2. Plugins can be reordered by some metadata in plugin description. Like a blacklist. But it requires good feedback from users.
  3. Some event based API, so all plugins will works together. Most complicated and promising way. It fix loop problem and should be faster. But not all plugins can use it (some plugins like Autoprefixer requires look back).
@ben-eb

This comment has been minimized.

Show comment
Hide comment
@ben-eb

ben-eb Apr 13, 2015

Member

I think finding a way for plugins to hook into certain events will be beneficial; for a lot of my modules, they do a lot of the same types of ground work to manipulate values. For example, with a rule like this:

h1 {
    margin: 10px 20px 10px 20px;
}

The plugin does not know beforehand if any of the values can be transformed through it, so it has to map each value through list.space, and for some properties, list.comma, then apply the transformation. So my plugins postcss-colormin and postcss-convert-values both have to apply this same logic to split up the value and transform it.

I believe that we can achieve a speed up if there was some way to hook this. 😄

However I would still like to be able to specify plugin order. So we need to be careful of not giving the developers enough hooks to register their modules into.

Member

ben-eb commented Apr 13, 2015

I think finding a way for plugins to hook into certain events will be beneficial; for a lot of my modules, they do a lot of the same types of ground work to manipulate values. For example, with a rule like this:

h1 {
    margin: 10px 20px 10px 20px;
}

The plugin does not know beforehand if any of the values can be transformed through it, so it has to map each value through list.space, and for some properties, list.comma, then apply the transformation. So my plugins postcss-colormin and postcss-convert-values both have to apply this same logic to split up the value and transform it.

I believe that we can achieve a speed up if there was some way to hook this. 😄

However I would still like to be able to specify plugin order. So we need to be careful of not giving the developers enough hooks to register their modules into.

@1j01

This comment has been minimized.

Show comment
Hide comment
@1j01

1j01 Apr 13, 2015

So function plugins could call the value hook on the parameters of function calls it finds, which would apply all the plugins that manipulate values, and the function plugin gets the result with nested function calls evaluated to send to whatever function. Assuming recursion is handled gracefully, that could work!

1j01 commented Apr 13, 2015

So function plugins could call the value hook on the parameters of function calls it finds, which would apply all the plugins that manipulate values, and the function plugin gets the result with nested function calls evaluated to send to whatever function. Assuming recursion is handled gracefully, that could work!

@ben-eb

This comment has been minimized.

Show comment
Hide comment
@ben-eb

ben-eb Apr 13, 2015

Member

Exactly, @1j01. 😃

Member

ben-eb commented Apr 13, 2015

Exactly, @1j01. 😃

@1j01

This comment has been minimized.

Show comment
Hide comment
@1j01

1j01 Apr 14, 2015

Sounds great to me! 👍
So. Plugins register hooks, and users will still need to be able to order them correctly, so hooks are applied in the order from the plugins list.
What hooks will there be, and what will the API look like?

1j01 commented Apr 14, 2015

Sounds great to me! 👍
So. Plugins register hooks, and users will still need to be able to order them correctly, so hooks are applied in the order from the plugins list.
What hooks will there be, and what will the API look like?

@ben-eb

This comment has been minimized.

Show comment
Hide comment
@ben-eb

ben-eb Apr 15, 2015

Member

I'm not sure on exactly which hooks we should implement just yet, but I really like the idea of having a streaming transform to handle this, ala browserify.

Of course, doing it this way may end up changing a lot of PostCSS's internals and API, so I would be cautious to recommend this as I know that PostCSS developers are already looking for API stabilisation. But, let me illustrate with a potential rewrite of https://github.com/ben-eb/postcss-discard-comments:

pipeline.get('comments').push(through.obj(function (node, enc, next) {
    if (!remover.canRemove(node)) {
        this.push(node);
    }
    next();
}));

As you can see, this is not using any APIs such as node#remove - it is just not including the comment in the stream. That way, other plugins will not see that comment node at all, and because the pipeline is an array of streaming transforms, you could insert other plugins before or after it with unshift, push or splice.

Member

ben-eb commented Apr 15, 2015

I'm not sure on exactly which hooks we should implement just yet, but I really like the idea of having a streaming transform to handle this, ala browserify.

Of course, doing it this way may end up changing a lot of PostCSS's internals and API, so I would be cautious to recommend this as I know that PostCSS developers are already looking for API stabilisation. But, let me illustrate with a potential rewrite of https://github.com/ben-eb/postcss-discard-comments:

pipeline.get('comments').push(through.obj(function (node, enc, next) {
    if (!remover.canRemove(node)) {
        this.push(node);
    }
    next();
}));

As you can see, this is not using any APIs such as node#remove - it is just not including the comment in the stream. That way, other plugins will not see that comment node at all, and because the pipeline is an array of streaming transforms, you could insert other plugins before or after it with unshift, push or splice.

@MoOx

This comment has been minimized.

Show comment
Hide comment
@MoOx

MoOx Apr 15, 2015

Member

Just don't do some over engineering for lazy people that do not think just a minute or read some documentation.
One example of people that doesn't think more than 20 seconds: postcss/postcss-bem-linter#37

Member

MoOx commented Apr 15, 2015

Just don't do some over engineering for lazy people that do not think just a minute or read some documentation.
One example of people that doesn't think more than 20 seconds: postcss/postcss-bem-linter#37

@ben-eb

This comment has been minimized.

Show comment
Hide comment
@ben-eb

ben-eb Apr 15, 2015

Member

@MoOx Indeed, but having a labelled pipeline would solve this problem of people using a linter before some non-standard syntax is compiled. The bem-linter plugin could hook into a validate label, which should be run after all values have been transformed.

Member

ben-eb commented Apr 15, 2015

@MoOx Indeed, but having a labelled pipeline would solve this problem of people using a linter before some non-standard syntax is compiled. The bem-linter plugin could hook into a validate label, which should be run after all values have been transformed.

@MoOx

This comment has been minimized.

Show comment
Hide comment
@MoOx

MoOx Apr 15, 2015

Member

Who decide when the css is valid or not ? :)

Member

MoOx commented Apr 15, 2015

Who decide when the css is valid or not ? :)

@ben-eb

This comment has been minimized.

Show comment
Hide comment
@ben-eb

ben-eb Apr 15, 2015

Member

By default, PostCSS should not be responsible itself for validating CSS. But, plugins can decide whether CSS is valid or not, and some kind of label/hook could be provided for this use case.

Member

ben-eb commented Apr 15, 2015

By default, PostCSS should not be responsible itself for validating CSS. But, plugins can decide whether CSS is valid or not, and some kind of label/hook could be provided for this use case.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Apr 16, 2015

Member

@MoOx yeap, I think too that we should have old object AST API aat least for custom in-company plugins.

And of course this features will be implemented only in 5.1+ versions. Maybe not in this year.

Member

ai commented Apr 16, 2015

@MoOx yeap, I think too that we should have old object AST API aat least for custom in-company plugins.

And of course this features will be implemented only in 5.1+ versions. Maybe not in this year.

@1j01

This comment has been minimized.

Show comment
Hide comment
@1j01

1j01 Apr 17, 2015

solve this problem of people using a linter before some non-standard syntax is compiled

@ben-eb Why would you/could you not just put it at the end where it should go?

1j01 commented Apr 17, 2015

solve this problem of people using a linter before some non-standard syntax is compiled

@ben-eb Why would you/could you not just put it at the end where it should go?

@ben-eb

This comment has been minimized.

Show comment
Hide comment
@ben-eb

ben-eb Apr 17, 2015

Member

Well in such a system that I described, the user is not responsible for deciding the order of the plugins, that is delegated to the plugin developer. So for example postcss-pseudoelements and postcss-minify-selectors both have responsibility for modifying selectors, but the minify module should come last, to ensure the selectors are compressed properly.

Therefore in this system I could push the minify selectors module to the bottom of the stack, and other developers could unshift their selector modules to the top. That way when a user wants to use the plugins they don't have to care about order, it is just abstracted away.

Of course, currently yes you can specify the order and transforms will be applied like that. It makes sense as that's how things like gulp work. I'm just presenting an alternative way to construct this pipeline of transforms. 😄

Member

ben-eb commented Apr 17, 2015

Well in such a system that I described, the user is not responsible for deciding the order of the plugins, that is delegated to the plugin developer. So for example postcss-pseudoelements and postcss-minify-selectors both have responsibility for modifying selectors, but the minify module should come last, to ensure the selectors are compressed properly.

Therefore in this system I could push the minify selectors module to the bottom of the stack, and other developers could unshift their selector modules to the top. That way when a user wants to use the plugins they don't have to care about order, it is just abstracted away.

Of course, currently yes you can specify the order and transforms will be applied like that. It makes sense as that's how things like gulp work. I'm just presenting an alternative way to construct this pipeline of transforms. 😄

@MoOx

This comment has been minimized.

Show comment
Hide comment
@MoOx

MoOx Apr 17, 2015

Member

If anyone try to fix pseudo element after some minifications, he is weird.
Something like minifications will always be at the end, we do not need over engineering for that.

developers could unshift their selector modules to the top

Maybe not at the top top of the top since stuff like import or some plugin that might generate selectors must be before. That would introduce very complicated shit to handle. Something that, as a plugin developer, I don't want to handle at all.

The user need to thinks a minute. Reminder: user is a developer.

Member

MoOx commented Apr 17, 2015

If anyone try to fix pseudo element after some minifications, he is weird.
Something like minifications will always be at the end, we do not need over engineering for that.

developers could unshift their selector modules to the top

Maybe not at the top top of the top since stuff like import or some plugin that might generate selectors must be before. That would introduce very complicated shit to handle. Something that, as a plugin developer, I don't want to handle at all.

The user need to thinks a minute. Reminder: user is a developer.

@ben-eb

This comment has been minimized.

Show comment
Hide comment
@ben-eb

ben-eb Apr 17, 2015

Member

Yeah, I'm not even sure if this is the right way to go, it's just how browserify handle it. I think if I am to push this idea any further then I would have to do some kind of proof of concept, to see if it is worth developing. Would like to hear any alternative suggestions, of course.

Member

ben-eb commented Apr 17, 2015

Yeah, I'm not even sure if this is the right way to go, it's just how browserify handle it. I think if I am to push this idea any further then I would have to do some kind of proof of concept, to see if it is worth developing. Would like to hear any alternative suggestions, of course.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai May 15, 2015

Member

Stylecow has nice event-based API.

Member

ai commented May 15, 2015

Stylecow has nice event-based API.

@1j01

This comment has been minimized.

Show comment
Hide comment
@1j01

1j01 May 16, 2015

@ai The API seems to be done in terms of "tasks" with "filters", not exactly "events", although I see how it does handle this sort of problem. It could definitely use some API sugar.
And its approach to plugins is horrible, bundling a bunch of modules into core. Anyways.

I've done a bit of a proof of concept here, also not exactly using "events" but rather "hooks"; and really it should be tied to the PostCSS instance, and the function implementation should handle recursion like rework-plugin-function does etc. but it works more or less (for my weird use case).

1j01 commented May 16, 2015

@ai The API seems to be done in terms of "tasks" with "filters", not exactly "events", although I see how it does handle this sort of problem. It could definitely use some API sugar.
And its approach to plugins is horrible, bundling a bunch of modules into core. Anyways.

I've done a bit of a proof of concept here, also not exactly using "events" but rather "hooks"; and really it should be tied to the PostCSS instance, and the function implementation should handle recursion like rework-plugin-function does etc. but it works more or less (for my weird use case).

@codemakerlive

This comment has been minimized.

Show comment
Hide comment
@codemakerlive

codemakerlive Aug 18, 2015

I haven't looked much at PostCSS internals, so perhaps you do this already... but how about a plugin-category-based pipeline internally handled within PostCSS, with plugin developer's ensuring they register a plugin against a category. PostCSS internally ensures that certain categories run at a specific stage of PostCSS's pipeline?

E.g. What babel does with it's transformers: https://github.com/babel/babel/blob/master/packages/babel/src/transformation/transformers/index.js

Granted babel will change in v6.0 to have plugin usage externalised and manually specified, but the system still provides some structure to ensure plugins execute at the correct point in time - e.g. Minification/Optimisation plug-ins ("optimisers") run last (or nearly last, perhaps plugins that post evaluate but perform no transforms ("post_evaluators") run last).

codemakerlive commented Aug 18, 2015

I haven't looked much at PostCSS internals, so perhaps you do this already... but how about a plugin-category-based pipeline internally handled within PostCSS, with plugin developer's ensuring they register a plugin against a category. PostCSS internally ensures that certain categories run at a specific stage of PostCSS's pipeline?

E.g. What babel does with it's transformers: https://github.com/babel/babel/blob/master/packages/babel/src/transformation/transformers/index.js

Granted babel will change in v6.0 to have plugin usage externalised and manually specified, but the system still provides some structure to ensure plugins execute at the correct point in time - e.g. Minification/Optimisation plug-ins ("optimisers") run last (or nearly last, perhaps plugins that post evaluate but perform no transforms ("post_evaluators") run last).

@rtsao

This comment has been minimized.

Show comment
Hide comment
@rtsao

rtsao Sep 1, 2015

Contributor

@1j01 It seems to me there's no reason those bundled plugins couldn't be taken out of the core; I'm not sure I'd go as far to say Stylecow's approach to plugins is "horrible" on that basis.

Contributor

rtsao commented Sep 1, 2015

@1j01 It seems to me there's no reason those bundled plugins couldn't be taken out of the core; I'm not sure I'd go as far to say Stylecow's approach to plugins is "horrible" on that basis.

@1j01

This comment has been minimized.

Show comment
Hide comment
@1j01

1j01 Sep 1, 2015

@rtsao Maybe "horrible" was a strong word. I find it inferior... and yucky.

1j01 commented Sep 1, 2015

@rtsao Maybe "horrible" was a strong word. I find it inferior... and yucky.

@rtsao

This comment has been minimized.

Show comment
Hide comment
@rtsao

rtsao Sep 1, 2015

Contributor

@1j01 Are you referring to the task/filter system itself or something else?

I'm genuinely interested in hearing more about what specifically you think is inferior as from my naive perspective they seem fairly comparable.

Plugin Comparison
PostCSS: postcss-css-variables
Stylecow: stylecow-plugin-variables

Plugin Usage Comparison
PostCSS:

var postcss = require('postcss');
postcss([ require('plugin1')(), require('plugin2')() ])
    .process(css, { from: 'src/app.css', to: 'app.css' })
    .then(function (result) {
        fs.writeFileSync('app.css', result.css);
        if ( result.map ) fs.writeFileSync('app.css.map', result.map);
    });

Stylecow:

var stylecow = require('stylecow');
stylecow
    .use(require('plugin1'))
    .use(require('plugin2'));
var css = stylecow.parseFile('src/app.css');
fs.writeFileSync('app.css', stylecow.run(css).toString());
Contributor

rtsao commented Sep 1, 2015

@1j01 Are you referring to the task/filter system itself or something else?

I'm genuinely interested in hearing more about what specifically you think is inferior as from my naive perspective they seem fairly comparable.

Plugin Comparison
PostCSS: postcss-css-variables
Stylecow: stylecow-plugin-variables

Plugin Usage Comparison
PostCSS:

var postcss = require('postcss');
postcss([ require('plugin1')(), require('plugin2')() ])
    .process(css, { from: 'src/app.css', to: 'app.css' })
    .then(function (result) {
        fs.writeFileSync('app.css', result.css);
        if ( result.map ) fs.writeFileSync('app.css.map', result.map);
    });

Stylecow:

var stylecow = require('stylecow');
stylecow
    .use(require('plugin1'))
    .use(require('plugin2'));
var css = stylecow.parseFile('src/app.css');
fs.writeFileSync('app.css', stylecow.run(css).toString());
@1j01

This comment has been minimized.

Show comment
Hide comment
@1j01

1j01 Sep 1, 2015

@rtsao Just the fact that it includes plugins bundled into core as dependencies. Not commenting on the API with regard to requiring/using plugins. Also, PostCSS let's you do .use(require('plugin')) too, so it's not really about that.

1j01 commented Sep 1, 2015

@rtsao Just the fact that it includes plugins bundled into core as dependencies. Not commenting on the API with regard to requiring/using plugins. Also, PostCSS let's you do .use(require('plugin')) too, so it's not really about that.

@oscarotero

This comment has been minimized.

Show comment
Hide comment
@oscarotero

oscarotero Sep 1, 2015

Hi, I just want to clarify that stylecow plugins are not bundled into core. The stylecow-core is an independent package. The stylecow repository only join the core and the plugins with some development tools (watcher, browser live reload, etc). This is just a way to execute stylecow with no complications. But you can use directly the stylecow-core package with no plugins.

oscarotero commented Sep 1, 2015

Hi, I just want to clarify that stylecow plugins are not bundled into core. The stylecow-core is an independent package. The stylecow repository only join the core and the plugins with some development tools (watcher, browser live reload, etc). This is just a way to execute stylecow with no complications. But you can use directly the stylecow-core package with no plugins.

@1j01

This comment has been minimized.

Show comment
Hide comment
@1j01

1j01 Sep 3, 2015

@oscarotero Okay, so there is a core without these dependencies, but that's not the recommended usage. You're recommended to use the stylecow CLI or Grunt/Gulp plugins, which bless a certain set of plugins. It creates a barrier to ecosystem development. That said, Stylecow like it says on the website has a "main purpose", so it's understandable why it would have built in plugins.

1j01 commented Sep 3, 2015

@oscarotero Okay, so there is a core without these dependencies, but that's not the recommended usage. You're recommended to use the stylecow CLI or Grunt/Gulp plugins, which bless a certain set of plugins. It creates a barrier to ecosystem development. That said, Stylecow like it says on the website has a "main purpose", so it's understandable why it would have built in plugins.

@MoOx

This comment has been minimized.

Show comment
Hide comment
@MoOx

MoOx Sep 3, 2015

Member

Did anyone check how Babel handle plugin order? If not you should.

Member

MoOx commented Sep 3, 2015

Did anyone check how Babel handle plugin order? If not you should.

@1j01

This comment has been minimized.

Show comment
Hide comment
@1j01

1j01 Sep 3, 2015

By default, plugins are run before the internal ones. You can alternatively specify the position via a colon after the plugin name. ie:

require("babel").transform("code", {
  plugins: ["foo-bar:after", "bar-foo:before"]
});

@MoOx It seems awkward. It's much better with PostCSS because there's nothing that's core, so you'd just do e.g. ["foo-bar", "postcss-babel", "bar-foo"] and wouldn't be able to say "after" before "before" like in the example.

1j01 commented Sep 3, 2015

By default, plugins are run before the internal ones. You can alternatively specify the position via a colon after the plugin name. ie:

require("babel").transform("code", {
  plugins: ["foo-bar:after", "bar-foo:before"]
});

@MoOx It seems awkward. It's much better with PostCSS because there's nothing that's core, so you'd just do e.g. ["foo-bar", "postcss-babel", "bar-foo"] and wouldn't be able to say "after" before "before" like in the example.

@MoOx

This comment has been minimized.

Show comment
Hide comment
@MoOx

MoOx Sep 3, 2015

Member

babel 6.0 will have nothing in it's core. With that in mind, think about that solution again.

Member

MoOx commented Sep 3, 2015

babel 6.0 will have nothing in it's core. With that in mind, think about that solution again.

@MoOx

This comment has been minimized.

Show comment
Hide comment
@MoOx
Member

MoOx commented Sep 3, 2015

@ai ai changed the title from Plugin conflicts to Event based API Nov 17, 2015

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Nov 17, 2015

Member

Maybe we should also have enter and exit events like in Babel 6.0 /via @ben-eb

Member

ai commented Nov 17, 2015

Maybe we should also have enter and exit events like in Babel 6.0 /via @ben-eb

@ai ai referenced this issue Nov 19, 2015

Closed

string interpolation #33

@ai ai added this to the 5.1 milestone Feb 22, 2016

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Feb 22, 2016

Member

I think we should have more people who familiar with PostCSS core internals. Also @marcustisater showed great example of “project technical committee” on postcss.org project.

So, let’s try same “technical committee” way in this feature. We need some leader, who want to coordinate the work. Who want to do it?

@ben-eb had experience with event-based API in cssnano 4 development. @jonathantneal thought about some proof-of-concept implementation.

Member

ai commented Feb 22, 2016

I think we should have more people who familiar with PostCSS core internals. Also @marcustisater showed great example of “project technical committee” on postcss.org project.

So, let’s try same “technical committee” way in this feature. We need some leader, who want to coordinate the work. Who want to do it?

@ben-eb had experience with event-based API in cssnano 4 development. @jonathantneal thought about some proof-of-concept implementation.

@marcustisater

This comment has been minimized.

Show comment
Hide comment
@marcustisater

marcustisater Feb 23, 2016

Member

Thank you Andrey, that's the beauty of Open Source and the PostCSS community.

It's going to take up a lot of time, planning and effort to organize an committee for such a big project but maybe it's worth the time to do that. I definitely think someone who has been involved a long time with PostCSS should take on the leader role (maybe the founder himself? 😉)

I would be happy to help with any documentation or co-help with coordinated work if needed.

Member

marcustisater commented Feb 23, 2016

Thank you Andrey, that's the beauty of Open Source and the PostCSS community.

It's going to take up a lot of time, planning and effort to organize an committee for such a big project but maybe it's worth the time to do that. I definitely think someone who has been involved a long time with PostCSS should take on the leader role (maybe the founder himself? 😉)

I would be happy to help with any documentation or co-help with coordinated work if needed.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Mar 7, 2016

Member

We are discussed with @ben-eb node changing API.

First option:

css.on('decl', function (decl) {
    if ( decl.prop === 'bad' ) {
        return false; // remove
    }
})

Second option:

css.on('decl', function (decl) {
    if ( decl.prop === 'bad' ) {
        decl.remove();
    }
})

We should think about node changes API because we should recall all node’s listeners on new appended nodes too.

Member

ai commented Mar 7, 2016

We are discussed with @ben-eb node changing API.

First option:

css.on('decl', function (decl) {
    if ( decl.prop === 'bad' ) {
        return false; // remove
    }
})

Second option:

css.on('decl', function (decl) {
    if ( decl.prop === 'bad' ) {
        decl.remove();
    }
})

We should think about node changes API because we should recall all node’s listeners on new appended nodes too.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Mar 7, 2016

Member

Good comment from @thejameskyle about their experience in Babel is relevant for this issue too #754 (comment)

Member

ai commented Mar 7, 2016

Good comment from @thejameskyle about their experience in Babel is relevant for this issue too #754 (comment)

@ben-eb

This comment has been minimized.

Show comment
Hide comment
@ben-eb

ben-eb Mar 7, 2016

Member

I think a visitors API like Babel is nicer for parser work than an event based API.

export default {
    decl: {
        enter: node => {
            if (node.prop === 'bad') {
                node.remove();
            }
        }
    }
}
Member

ben-eb commented Mar 7, 2016

I think a visitors API like Babel is nicer for parser work than an event based API.

export default {
    decl: {
        enter: node => {
            if (node.prop === 'bad') {
                node.remove();
            }
        }
    }
}
@jamiebuilds

This comment has been minimized.

Show comment
Hide comment
@jamiebuilds

jamiebuilds Mar 7, 2016

Moving from the existing api to this events api to a visitor api is non-linear, I think you could introduce a visitor-based api on top of what exists today without diving too deep on parsing a more detailed AST.

I would recommend against doing this as it's just api thrashing if you plan on moving to a visitor api later

jamiebuilds commented Mar 7, 2016

Moving from the existing api to this events api to a visitor api is non-linear, I think you could introduce a visitor-based api on top of what exists today without diving too deep on parsing a more detailed AST.

I would recommend against doing this as it's just api thrashing if you plan on moving to a visitor api later

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Mar 7, 2016

Member

@ben-eb do we have any benefits with object-style API (when you put events name as object keys), rather than listener API (when you can a .on() method)?

Member

ai commented Mar 7, 2016

@ben-eb do we have any benefits with object-style API (when you put events name as object keys), rather than listener API (when you can a .on() method)?

@jamiebuilds

This comment has been minimized.

Show comment
Hide comment
@jamiebuilds

jamiebuilds Mar 7, 2016

Visitors is a well defined pattern that you can reuse easily. Creating multiple visitors and performing traversals in not uncommon. For an example see this bit about handling state in Babel.

Also, visitors are much easier to optimize as they are a static thing. Babel has an optimization where it takes the primary visitors from every single plugin and merges them into a single visitor that only needs to traverse the tree once.

jamiebuilds commented Mar 7, 2016

Visitors is a well defined pattern that you can reuse easily. Creating multiple visitors and performing traversals in not uncommon. For an example see this bit about handling state in Babel.

Also, visitors are much easier to optimize as they are a static thing. Babel has an optimization where it takes the primary visitors from every single plugin and merges them into a single visitor that only needs to traverse the tree once.

@Termina1

This comment has been minimized.

Show comment
Hide comment
@Termina1

Termina1 Apr 26, 2016

Contributor

Yet, babel plugins order is unpredictable (example).

It has good optimisations trying to merge visitors, but I personally don't think it is the best approach as a base case, because every time you have 2 plugins that want to modify same AST parts, the order of them is hidden inside AST and plugins code itself.

In my opinion, the order of plugins should be obvious for a user and the most obvious thing is the order of plugins in the config array. Most of the time, you don't want speed as PostCSS is fast enough, but when you do, may be we could have some layer on top of postcss, which would implement the babel-like plugin syntax and navigation for cases, when we want to minimise the amount of tree walks?

As I understood, @thejameskyle suggested this approach in hist first comment?

Contributor

Termina1 commented Apr 26, 2016

Yet, babel plugins order is unpredictable (example).

It has good optimisations trying to merge visitors, but I personally don't think it is the best approach as a base case, because every time you have 2 plugins that want to modify same AST parts, the order of them is hidden inside AST and plugins code itself.

In my opinion, the order of plugins should be obvious for a user and the most obvious thing is the order of plugins in the config array. Most of the time, you don't want speed as PostCSS is fast enough, but when you do, may be we could have some layer on top of postcss, which would implement the babel-like plugin syntax and navigation for cases, when we want to minimise the amount of tree walks?

As I understood, @thejameskyle suggested this approach in hist first comment?

@jamiebuilds

This comment has been minimized.

Show comment
Hide comment
@jamiebuilds

jamiebuilds Apr 26, 2016

I personally don't think it is the best approach as a base case, because every time you have 2 plugins that want to modify same AST parts, the order of them is hidden inside AST and plugins code itself.

If a plugin is so sensitive that it breaks when the AST gets modified between visitors calls, it's almost always (99.9999% of the time) because the transformation has been written poorly. It's likely holding onto state or making assumptions about the AST.

This approach has actually forced plugin authors to write better quality transformations.

jamiebuilds commented Apr 26, 2016

I personally don't think it is the best approach as a base case, because every time you have 2 plugins that want to modify same AST parts, the order of them is hidden inside AST and plugins code itself.

If a plugin is so sensitive that it breaks when the AST gets modified between visitors calls, it's almost always (99.9999% of the time) because the transformation has been written poorly. It's likely holding onto state or making assumptions about the AST.

This approach has actually forced plugin authors to write better quality transformations.

@Termina1

This comment has been minimized.

Show comment
Hide comment
@Termina1

Termina1 Apr 27, 2016

Contributor

@thejameskyle I have a good example: babel-plugin-transform-flow-strip-types, which strips flow types and runs almost always before any other plugin, essentially making any other plugin that want to use flow types useless. It's hard to find out, hard to debug and super unobvious due to the babel plugin running strategy. The only solution as far as I know is option passPerPreset.

The problem is not that plugins are fragile, the problem is that transformations in some cases remove some information (whether implicitly removing it or making it harder to obtain), and you will never know in general case if there is some plugin that wants that information, but is set to be evaluated later that your destructive plugin.

Contributor

Termina1 commented Apr 27, 2016

@thejameskyle I have a good example: babel-plugin-transform-flow-strip-types, which strips flow types and runs almost always before any other plugin, essentially making any other plugin that want to use flow types useless. It's hard to find out, hard to debug and super unobvious due to the babel plugin running strategy. The only solution as far as I know is option passPerPreset.

The problem is not that plugins are fragile, the problem is that transformations in some cases remove some information (whether implicitly removing it or making it harder to obtain), and you will never know in general case if there is some plugin that wants that information, but is set to be evaluated later that your destructive plugin.

@jamiebuilds

This comment has been minimized.

Show comment
Hide comment
@jamiebuilds

jamiebuilds Apr 27, 2016

If you want that data it should be added to the AST as metadata at the start, not left in as AST nodes. The problem _is_ that the plugins are more fragile than they should be.

Babel hasn't spent a lot of time working to make the flow experience awesome so things like that are left out.

jamiebuilds commented Apr 27, 2016

If you want that data it should be added to the AST as metadata at the start, not left in as AST nodes. The problem _is_ that the plugins are more fragile than they should be.

Babel hasn't spent a lot of time working to make the flow experience awesome so things like that are left out.

@Termina1

This comment has been minimized.

Show comment
Hide comment
@Termina1

Termina1 Apr 27, 2016

Contributor

@thejameskyle the AST itself is an information that is being partially lost during transformations

Contributor

Termina1 commented Apr 27, 2016

@thejameskyle the AST itself is an information that is being partially lost during transformations

@jamiebuilds

This comment has been minimized.

Show comment
Hide comment
@jamiebuilds

jamiebuilds Apr 27, 2016

Not if you store it properly

jamiebuilds commented Apr 27, 2016

Not if you store it properly

@ai ai modified the milestones: 6.0, 5.1 May 3, 2016

@ai ai referenced this issue Sep 26, 2016

Closed

Plugin integration #895

@ai ai removed this from the 6.0 milestone Apr 14, 2017

@ai ai referenced this issue Jun 11, 2017

Closed

Recursive mixins #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment