Add postprocess plugin set #60

alexlafroscia · 2017-01-03T21:24:37Z

Adds a new option to provide a set of PostCSS plugins to run after concatenation has taken place. This is useful for plugins like postcss-sprites, which should be run later in the CSS pipeline.

Closes salsify/broccoli-css-modules#3

Todo

~~Unit tests for the new Broccoli plugin~~ replaced by a dependency
Rebase from master
Smoke test to ensure the postprocess plugins are applied to the output file.

dfreeman · 2017-01-04T15:19:06Z

I probably won't have a chance to look at this deeply until this weekend, but thank you so much for taking this on!

alexlafroscia · 2017-01-05T01:20:06Z

No problem 😄 we're just using my fork in our app right now and I can confirm it works as intended. I'm excited to get this finished and published, we've been using a fork of ember-css-modules for a long time and I'm stoked to be able to just track the latest from here on out.

dfreeman

A couple questions, but overall this is looking great — let me know if there's anything I can do to help out on the testing front.

dfreeman · 2017-01-08T22:29:48Z

lib/output-styles-preprocessor.js

@@ -24,8 +25,17 @@ OutputStylesPreprocessor.prototype.toTree = function(inputNode, inputPath, outpu

  debug('concatenating module stylesheets: %o', concatOptions);

+  var parentOptions = options.registry.app.options || {};
+  var cssModulesOptions = parentOptions.cssModules || {};
+  var postprocessPlugins = cssModulesOptions.plugins && cssModulesOptions.plugins.postprocess;


You can call this.owner.getPlugins() to simplify this

Oh awesome, I'll update that.

@dfreeman was this API removed between the initial PR and now? That method doesn't exist anymore

edit would the right thing now be getPostcssPlugins? That seems to do what I'm looking for

I think that's right—IIRC that was renamed to distinguish between ECM plugins and PostCSS plugins

dfreeman · 2017-01-08T22:31:25Z

lib/postcss-persistent-filter.js

+    map: {
+      inline: false,
+      annotation: false
+    }


Will this play nicely with user-specified source map settings?

Not sure -- @jasonmit ?

Unsure what is meant by user specific map settings, these: https://ember-cli.com/asset-compilation#source-maps ?

Yep, we read that config and pass it along to broccoli-css-modules (as well as using it when concatenating everything together)

dfreeman · 2017-01-08T22:31:39Z

package.json

@@ -24,6 +24,7 @@
  },
  "homepage": "https://github.com/salsify/ember-css-modules#readme",
  "devDependencies": {
+    "broccoli-persistent-filter": "^1.2.11",


This should be in dependencies, right?

Hmm, probably. @jasonmit ?

Yes, should be moved

alexlafroscia · 2017-01-11T20:34:15Z

Sorry this got away from me @dfreeman. If you know of a good way to do the smoke test for the postprocess set, setting that up would be extremely helpful. I'll handle the unit test for the broccoli plugin tonight to make sure that it works right.

This is intended only for short term use / testing. Will be unnecessary once salsify#60 and broccoli-css-modules#4 are merged

FransTwisk · 2017-06-27T07:11:01Z

I hope this will be merged eventually. Very useful.

alexlafroscia · 2017-06-27T20:45:23Z

Yeah, sorry about that. getting back to the tests just hasn't been a priority but should be. It won't be this weekend, but maybe the weekend after I'll be able to carve out some time

FransTwisk · 2017-07-20T10:23:32Z

Any updates on this?

alexlafroscia · 2017-07-20T17:43:37Z

Yeah, so... No. But I really need to do this, since our app requires this PR and we'd really like to get on the official release (and PostCSS 6). My holdup is mostly just not really knowing how to test Broccoli stuff. Maybe @jasonmit has some insight there?

dfreeman · 2017-07-20T19:11:17Z

For testing Broccoli stuff I've found broccoli-fixture to be super helpful — we use it over in broccoli-css-modules, though that project is still on Mocha instead of QUnit for its tests.

alexlafroscia · 2017-07-21T18:01:37Z

Cool, I'll definitely check into that. First step is rebasing, since it's been so long since this PR was made 😅 but it's on my list to check this out this weekend since we really want to get our app off of PostCSS 5 and onto 6.

alexlafroscia · 2017-07-23T22:06:37Z

@dfreeman after checking out the new Plugin system you have added since this PR started (which is awesome, by the way!) I think this could actually be added as a plugin instead. Would you prefer that to adding it to the default add-on? I feel like the use-cases for this are actually pretty slim, and like the idea of allowing people to opt in to running the code by installing an additional add-on.

I think it could look a lot like the Sass plugin, just running arbitrary PostCSS plugins that the user can configure instead of enabling Sass.

edit after looking more... maybe that's not the right approach. I'm going to keep working on this as a PR.

dfreeman · 2017-07-23T22:43:00Z

@alexlafroscia I think framing this as a plugin is an interesting idea — as it sounds like you've discovered, the current plugin API is lacking the hook you'd need, but I could easily imagine adding broccoli-based plugin methods for manipulating the tree pre-CSS-modules, post-CSS-modules and post-concatenation.

Your plugin would just implement that third hook, taking the concatenated file and performing an additional round of PostCSS processing on it. What do you think of an approach like that?

alexlafroscia · 2017-07-23T22:51:14Z

Yeah, that sounds interesting! I'm fine doing it that way.

My first commit here (I squashed all the previous commits together to make it easier to rebase onto master) implements things basically the same way that the initial PR did. The only major difference is that instead of including the Broccoli plugin that will get run on post-process as part of this repo, I found a separate one that was built on the same broccoli-persistent-filter that was already tested, and used that instead.

dfreeman · 2017-07-23T22:55:48Z

Given how lightweight this wound up being, I'm not opposed to landing it as core functionality, as long as we can get some kind of smoke test in place in one of the dummy app/addons to prevent regressions.

Regardless, it does feel like plugins should be able to implement something like this, but I can save tackling that for another day :)

alexlafroscia · 2017-07-23T22:57:55Z

Working on that test now :) Just trying to look through the existing tests to get an idea of how I can verify that the postprocess plugins are actually run.

alexlafroscia · 2017-07-23T23:32:26Z

@dfreeman what would you be looking for for a "smoke test" here? I'm not sure how to verify that those plugins were run other than setting up a dummy app with a plugin and checking to see if the output CSS was transformed. Would that do? Maybe using ember-cli-addon-tests?

dfreeman · 2017-07-23T23:38:27Z

I think the simplest thing to do would be to add a component to the dummy app whose styles.css relies on something you configure as a postprocess plugin. Then you can just throw together an integration test to verify the right style actually gets applied, similar to how we verify that imports resolve correctly.

alexlafroscia · 2017-07-28T01:30:03Z

👍 great idea. I'll do that!

I made a PR there that enables the same style of plugin definition that the `before` and `after` hooks support to make `postprocess` consistent with them. This bumps the version of the dependency to include that change.

alexlafroscia · 2017-07-28T01:50:38Z

No idea if I did that right... I'm on a new laptop and was having trouble getting the tests running locally. I'm going to let CI tell me if that worked.

alexlafroscia · 2017-08-15T23:45:22Z

@dfreeman just wanted to bump this... we'd love to get off of my old fork and onto the latest version, but we need this to land first!

dfreeman · 2017-08-16T15:25:58Z

Oops, sorry for the delay @alexlafroscia! I meant to look at this before I left for vacation and totally lost track. I should have some time in the next day or so to take a look.

alexlafroscia · 2017-08-16T19:35:56Z

No problem! Thanks for merging, glad to get this landed after so long (my fault, not yours) 😅

dfreeman · 2017-08-16T20:44:37Z

This is now released in 0.7.3 — thanks again @alexlafroscia! 🎉

dfreeman reviewed Jan 8, 2017

View reviewed changes

Add set of plugins to run after concat

8f45c3c

alexlafroscia force-pushed the add-postprocess-plugin-set branch from db3ca50 to 8f45c3c Compare July 23, 2017 22:47

alexlafroscia mentioned this pull request Jul 23, 2017

Allowing an array of plugins instead of objects jeffjewiss/broccoli-postcss#114

Closed

Update docs on postprocess plugin set

b5b3c82

Update postcss-sprites

21a51cd

I made a PR there that enables the same style of plugin definition that the `before` and `after` hooks support to make `postprocess` consistent with them. This bumps the version of the dependency to include that change.

alexlafroscia force-pushed the add-postprocess-plugin-set branch from 1817a6b to 556bea0 Compare July 28, 2017 01:54

Add smoke test for postprocess plugins

0aad226

alexlafroscia force-pushed the add-postprocess-plugin-set branch from 556bea0 to 0aad226 Compare July 28, 2017 17:29

dfreeman merged commit 95fb7e0 into salsify:master Aug 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add postprocess plugin set #60

Add postprocess plugin set #60

alexlafroscia commented Jan 3, 2017 •

edited

dfreeman commented Jan 4, 2017

alexlafroscia commented Jan 5, 2017

dfreeman left a comment

dfreeman Jan 8, 2017

alexlafroscia Jan 11, 2017

alexlafroscia Jul 23, 2017 •

edited

dfreeman Jul 23, 2017

dfreeman Jan 8, 2017

alexlafroscia Jan 11, 2017

jasonmit Apr 12, 2017

dfreeman Apr 12, 2017 •

edited

dfreeman Jan 8, 2017

alexlafroscia Jan 11, 2017

jasonmit Apr 12, 2017

alexlafroscia commented Jan 11, 2017

FransTwisk commented Jun 27, 2017

alexlafroscia commented Jun 27, 2017

FransTwisk commented Jul 20, 2017

alexlafroscia commented Jul 20, 2017

dfreeman commented Jul 20, 2017

alexlafroscia commented Jul 21, 2017

alexlafroscia commented Jul 23, 2017 •

edited

dfreeman commented Jul 23, 2017

alexlafroscia commented Jul 23, 2017

dfreeman commented Jul 23, 2017

alexlafroscia commented Jul 23, 2017

alexlafroscia commented Jul 23, 2017

dfreeman commented Jul 23, 2017

alexlafroscia commented Jul 28, 2017

alexlafroscia commented Jul 28, 2017 •

edited

alexlafroscia commented Aug 15, 2017

dfreeman commented Aug 16, 2017

alexlafroscia commented Aug 16, 2017

dfreeman commented Aug 16, 2017

Add postprocess plugin set #60

Add postprocess plugin set #60

Conversation

alexlafroscia commented Jan 3, 2017 • edited

Todo

dfreeman commented Jan 4, 2017

alexlafroscia commented Jan 5, 2017

dfreeman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexlafroscia Jul 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfreeman Apr 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexlafroscia commented Jan 11, 2017

FransTwisk commented Jun 27, 2017

alexlafroscia commented Jun 27, 2017

FransTwisk commented Jul 20, 2017

alexlafroscia commented Jul 20, 2017

dfreeman commented Jul 20, 2017

alexlafroscia commented Jul 21, 2017

alexlafroscia commented Jul 23, 2017 • edited

dfreeman commented Jul 23, 2017

alexlafroscia commented Jul 23, 2017

dfreeman commented Jul 23, 2017

alexlafroscia commented Jul 23, 2017

alexlafroscia commented Jul 23, 2017

dfreeman commented Jul 23, 2017

alexlafroscia commented Jul 28, 2017

alexlafroscia commented Jul 28, 2017 • edited

alexlafroscia commented Aug 15, 2017

dfreeman commented Aug 16, 2017

alexlafroscia commented Aug 16, 2017

dfreeman commented Aug 16, 2017

alexlafroscia commented Jan 3, 2017 •

edited

alexlafroscia Jul 23, 2017 •

edited

dfreeman Apr 12, 2017 •

edited

alexlafroscia commented Jul 23, 2017 •

edited

alexlafroscia commented Jul 28, 2017 •

edited