Skip to content
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

Add support for postcss loader #17

Merged
merged 4 commits into from
Mar 15, 2016
Merged

Add support for postcss loader #17

merged 4 commits into from
Mar 15, 2016

Conversation

jonathanglasmeyer
Copy link
Collaborator

I added "skeletonic" support for the postcss property used by postcss-loader and added a webpack config of mine to verify acceptance.

This is a cool project, I'd like to get involved. 👍

**Why**: Make configs that use postcss-loader pass webpack-validator
and not emit the warning that `postcss` is an unknown property.
**Why**: It contains the `postcss` property, which belongs to
`postcss-loader`. This shouldn't emit a warning now, since the
`postcss` property was added to the accepted properties in the last
commit.
Although it is (still?) allowed to just specify an array, the only
documented form is specifying a function. (See https://github.com/postcss/postcss-loader).
It might be a good idea to enforce the usage of the function form with
the help of this validator.
**Why:** An initial stab at how validation should work (?): Steering the user
towards canonical use of the loader in question by prompting him/her to match
the format of the examples shown in the loader's README.

This clears up user's confusion wether the format in which they specified it is still
valid. The specific problem here was that I had specified the option
values of postcss-plugin as an array (`postcss: [...]`) while the
documentation showed them only as `postss: function() { return [...] }`.
It was thus unclear to me wether some major update of the loader would
break my build or if my format was still ok. Only by looking into the
loaders source could I see that my format would still be ok.
@kentcdodds
Copy link
Owner

Sorry it took so long for me to get to you on this one!

I love it and I'm glad you want to get involved!

I'm not sure how I want to handle 3rd party plugin options. Part of me wants to just include them by default, but another part of me wants to require people to explicitly import them.

What I mean is, people would have to do:

var validateWebpack = require('webpack-validator')

module.exports = validateWebpack(config, [
  validateWebpack.postcss,
  validateWebpack.eslint,
  // etc...
])

Thoughts? I can see benefits/drawbacks for both. 💭

@jonathanglasmeyer
Copy link
Collaborator Author

That's the crucial question. On one hand this is a tool to reduce tooling
fatigue, so like the baseline premise should be sensible defaults over
configuration for this thing. When I'm already fatigued I def will not care
to configure Yet Another Tool.
On the other hand I guess that we'd want plugin authors to be able to write
their own validators and make them pluggable into webpack-validator via
require 'webpack-validator-postcss-loader-plugin' or even require postcss-loader/validate.
A problem is see with this approach is how the full config integration
tests should be run when plugins are externalized.

I wonder if it's possible to find a middleground. So maybe start integrated
and move to externals later. Or intelligently find installed external
validators in node modules folder according to something like the proposed
naming schemes above and automatically use them, plus curate the existing
selection of external validators in a very visible way in the README?
Maybe even messages like "You seem to be using postcss loader. To validate
it, npm install webpack-validators-postcss-loader-plugin". (If the plugin
author exposes it as postcss-loader/validate, the integration could happen
completely automatically.)

Just some thoughts. :)

@kentcdodds
Copy link
Owner

I'm sold. Let's do this.

kentcdodds pushed a commit that referenced this pull request Mar 15, 2016
@kentcdodds kentcdodds merged commit 5e4a469 into kentcdodds:master Mar 15, 2016
@jonathanglasmeyer
Copy link
Collaborator Author

Do...what?😃
The formatting of my last message was weird (I sent it per mail). I've edited my last message so that it shows the whole text now.

@kentcdodds
Copy link
Owner

Yeah, noticed that after the fact :-)

I was sold when you said:

When I'm already fatigued I def will not care to configure Yet Another Tool.

One of the goals of this project is to reduce fatigue, not increase it!

@jonathanglasmeyer
Copy link
Collaborator Author

Ok so what does that imply for the plugin architecture I proposed? What if plugins are just required by webpack-validator itself and the user doesn't have to care about installing and configuring them? Wouldn't we get the best of two worlds: ease of setup while still being able to run ntegration tests including those blessed plugins; plus allowing webpack loader authors/contributors to maintain these validators without having to PR this project each time the plugin interface changes?

Finally, users could opt-in to supplying other, not-yet-blessed or custom plugins themselves.

@kentcdodds
Copy link
Owner

That'd be cool. I definitely would like to have a spec that said: "Check the user's package.json and if there's anything that starts with webpack-, attempt a require('webpack-whatever/validate'). That would be interesting. I'm definitely interested in a PR about that. Want to file an issue to track that?

@jonathanglasmeyer
Copy link
Collaborator Author

Will do. Probably will find time to work on it on the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants