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

V4 -- Config error on installing w/ PostCSS support #307

Open
sthzg opened this issue Nov 22, 2016 · 14 comments
Open

V4 -- Config error on installing w/ PostCSS support #307

sthzg opened this issue Nov 22, 2016 · 14 comments
Labels
Milestone

Comments

@sthzg
Copy link
Member

sthzg commented Nov 22, 2016

When installing with PostCSS support, the postcss config property seems to be inserted at the wrong place (the config fails webpack configuration).

Invalid configuration object. Webpack has been initialised using a configuration object that does not match the API schema.
 - configuration has an unknown property 'postcss'. These properties are valid:
   object { amd?, bail?, cache?, context?, dependencies?, devServer?, devtool?, entry, externals?, loader?, module?, name?, node?, output?, plugins?, profile?, recordsInputPath?, recordsOutputPath?, recordsPath?, resolve?, resolveLoader?, stats?, target?, watch?, watchOptions? }
   For typos: please correct them.
   For loader options: webpack 2 no longer allows custom properties in configuration.
     Loaders should be updated to allow passing options via loader options in module.rules.
     Until loaders are updated one can use the LoaderOptionsPlugin to pass these options to the loader:
     plugins: [
       new webpack.LoaderOptionsPlugin({
         // test: /\.xxx$/, // may apply this only for some modules
         options: {
           postcss: ...
         }
       })
     ]

/cc @stylesuxx

@sthzg sthzg added the bug label Nov 22, 2016
@sthzg sthzg added this to the 4.0 milestone Nov 22, 2016
@stylesuxx
Copy link
Contributor

Ahh, right - this property... I am on it.

@sthzg
Copy link
Member Author

sthzg commented Nov 23, 2016

@stylesuxx let me know if I can be of any assistance with testing/fixing this bug.

@weblogixx
Copy link
Member

@stylesuxx / @sthzg: Maybe we should add the default key and remove the question on installing if this is possible? Users would then only have to install the postcss node modules per hand and the config would not have to be altered as it is today. What do you think about this?

@sthzg
Copy link
Member Author

sthzg commented Nov 25, 2016

@weblogixx just a short follow-up: @stylesuxx came up w/ a PR that fully integrates the PostCSS functionality again #313. The only thing left to merge this PR in is a final, manual check.

@weblogixx
Copy link
Member

Hi @sthzg,

nice, will also have a look at it. I really dislike the way we are reparsing the config with esprima in the current version, just wanted to remove this if it is possible.

@stylesuxx
Copy link
Contributor

stylesuxx commented Nov 25, 2016 via email

@weblogixx
Copy link
Member

@stylesuxx, yes, that would be the way to go for me. In this case, we just would have to remove the question (add postcss support) when using v4 and up, so v3 would still implement it.

@sthzg
Copy link
Member Author

sthzg commented Nov 25, 2016

Cool, then I defer the manual testing until after these new changes.

sthzg added a commit that referenced this issue Dec 17, 2016
- bumped postcss and postcss-loader versions
- removed deprecated custom logic related to setting up PostCSS
- updated tests

As communicated in #307 and on the related PR from @stylesuxx on the
template repo we can use PostCSS` more recent config file feature
instead of modifying our config.

If users prompt the PostCSS question with yes, deps for postcss and
postcss-loader are added to their package.json.

The updated tests check that
- postcss and postcss-loader deps are added on PostCSS yes
- postcss and postcss-loader deps are not added on PostCSS no
- postcss.config.js is copied to the project root
@sthzg
Copy link
Member Author

sthzg commented Dec 17, 2016

@stylesuxx I started to prepare the gen-side in above's commit (cece8d0).

The error on Travis is on purpose, as the updated tests require removing the postcss and postcss-loader deps on the template repo (I've commented on the PR there).

Maybe you could also look over this PR. I am not sure if we could get rid of some of the deps that were necessary for AST processing in the postcss.js install step (now removed, some of the AST deps are required by the setup-env subgen though).

IMO we can now finish testing with the combination of

  • your PR version on the template repo and
  • this PR's version of the generator

and then maybe finally ship this thing 🙌 ...

@sthzg
Copy link
Member Author

sthzg commented Dec 17, 2016

Test Setup PostCSS Feature

  • PostCSS w/ installed autoprefixer, 5 versions backwards, check if vendor prefixes are added.
  • 3 levels of imports: app.css --> @imports --> one.css --> @imports --> two.css
  • imports should contain font imports and prepro features (i.e. for LESS and SCSS variables, nesting, and mixins)

Checks

  • .css, no css-modules imports, font import
  • .css, css-modules imports, font import
  • .less, no css-modules nesting, vars through import, mixin through import, font import
  • .less, css-modules nesting, vars through import, mixin through import, font import
  • .scss, no css-modulesnesting, vars through import, mixin through import, font import
  • .scss, css-modulesnesting, vars through import, mixin through import, font import
  • .styl, no css-modulesnesting, vars through import, mixin through import, font import
  • .styl, css-modulesnesting, vars through import, mixin through import, font import

@stylesuxx
Copy link
Contributor

Thank you so much for all your testing, I adapted the deps in the template. Should I to the 3 last checks or are you on it?

@sthzg
Copy link
Member Author

sthzg commented Dec 17, 2016

@stylesuxx only the stylus tests are left. one thing that puzzles me is that I wasn't able to recreate the problems I had w/ scss in a real-world project. my take on that is that I consider it working until I can distill the error out of that other project and am able to get a test-case up. I'll have enough time this evening to do both *.styl tests. when that works I think we can ping weblogixx to get a final verdict. :)

@sthzg
Copy link
Member Author

sthzg commented Dec 17, 2016

Awesome work! @weblogixx could you take look over both PRs?

  1. PR on template: Added postCSS functionality react-webpack-template#77
  2. PR on generator: Updated functionality related to PostCSS - #307 #319

Once 1) is merged and published I would trigger Travis on 2) again so that it (hopefully) turns green.

@weblogixx
Copy link
Member

Hi @sthzg and @stylesuxx,

just had a look at both prs. Seems really nice! Thank you for your continued work for this project. Keep up the good work! 👍

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

No branches or pull requests

3 participants