Use postcss.config.js when present #4411

kylemh · 2018-10-13T20:55:01Z

What I did

Add find-up to dependencies to search for the nearest postcss.config.js
Use the nearest postcss.config.js to determine postcss-loader options if present, maintain previous defaults if not.

How to test

Use local storybook core against a test repository using a postcss config with an option not present in the default set (such as postcss-import).

Is this testable with Jest or Chromatic screenshots? Only if a custom serializer renders all styles.
Does this need a new example in the kitchen sink apps?
No.

Does this need an update to the documentation?
Yes. Describe that custom postcss.config.js should be handled just like custom babel configurations. Just wanted to ensure this is the correct direction before I add any changes.

If your answer is yes to any of these, please make sure to include it in your PR.

For maintainers only: Please tag your pull request with at least one of the following:
["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

codecov · 2018-10-14T20:20:54Z

Codecov Report

Merging #4411 into master will decrease coverage by 0.05%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #4411      +/-   ##
==========================================
- Coverage   35.57%   35.52%   -0.06%     
==========================================
  Files         557      558       +1     
  Lines        6732     6742      +10     
  Branches      884      885       +1     
==========================================
  Hits         2395     2395              
- Misses       3877     3886       +9     
- Partials      460      461       +1

Impacted Files	Coverage Δ
lib/core/src/server/config/style.config.js	`0% <0%> (ø)`
...b/core/src/server/config/webpack.config.default.js	`0% <0%> (ø)`	⬆️
lib/core/src/server/core-preset-webpack-custom.js	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d16f1d6...dfa78c3. Read the comment docs.

igor-dv · 2018-10-14T21:12:35Z

lib/core/src/server/config/webpack.config.default.js

-              },
+              options: postcssConfig
+                ? {
+                    postcssConfig,


should it be { ...postcssConfig } ?

I think will be better something like this:

postcssConfig || { // flexbug blah blah }

I think the config would work without a spread operator, but adding it doesn't hurt anything, so I will 👍

I'm totally fine with that! I do my best to not subscribe to lint beliefs and simply adhere to the linting of the code base. I have seen @ndelangen express a favor towards ternaries when discussing short-circuits, so I did it that way; perhaps a lint rule is in order to prevent further confusion?

I was more about to simplify the readability. Maybe even worth to extract it to a func

simplify the readability

is subjective.

I think ternaries read fine, ~~but most importantly this discussion shouldn't even happen. We should just have a lint rule to prevent bike-shedding. I want to do whatever the team wants to do here.~~

Doesn't look like ESLint has a rule to prevent this discussion aside from preventing ternaries at all. It'd be cool to see a prefer-short-circuit lint rule which favors && or || instead of ternaries where one option is null or undefined

Anywho... I'll go ahead and make both of those changes.

igor-dv

We should have an example for this I think

igor-dv · 2018-10-14T21:14:13Z

lib/core/src/server/config/webpack.config.default.js

+                  }
+                : {
+                    plugins: () => [
+                      require('postcss-flexbugs-fixes'), // eslint-disable-line global-require


I understand that something won't work if this plugin will be missing?

The plugins are currently available, correct? I didn't remove dependencies for that reason.

I mean, should we somehow merge it to the user's config?

Oh, I wouldn't do that. Either we give them a set of defaults, or they have full control. I'm not a fan of doing obscure things like providing a pair of plugins the user may or may not want if they have their own postcss config.

kylemh · 2018-10-14T23:22:50Z

Agreed on needing an example. I'll add one and update docs as long as this is going in the right direction.

kylemh · 2018-10-17T05:53:24Z

Gonna wait on #4405 before I move forward.

The only work left to be done will be to integrate with those changes + provide an example + update docs 👍

I'm prepared to make the moves at any moment in my work day.

…/use-custom-postcss-config

storybook-safe-bot · 2018-10-25T08:14:18Z

	Fails
🚫	PR is marked with "feature request" label.

Generated by 🚫 dangerJS

igor-dv · 2018-10-25T09:33:13Z

lib/core/src/server/config/style.config.js

+
+export default function getStyleConfig(baseConfig) {
+  // null if file is not found
+  const customPostcssConfig = findUp.sync('postcss.config.js', {


Will be very nice if you could asyncify this... Need to add some additional awaits in the call chain though. Not critical one, bit since we already support async/await that be great =)

…/use-custom-postcss-config

kylemh · 2018-10-25T10:45:48Z

Suggested spot for documentation:
A section after https://storybook.js.org/configurations/custom-babel-config/ called "other common custom configurations" to leave room for how postcss.config.js gets consumed and - eventually - how .browserlistrc gets consumed

Norbert spoke with me about creating an example folder for this, so I will 👍

Temporarily committed example to make it easier for people to try out themselves.

In testing it locally, I run yarn build-storybook against CRA storybook example with a postcss.config.js (even though CRA doesnt support postcss.config) at the project root and with the rebecca purple import. I would expect the chunk to have that color value changed to an rgb() calc that results in #663399, but the name rebeccapurple remains... You should also be able to simply open the browser with yarn storybook and check out dev tools to be on the lookout for the same value change.

igor-dv · 2018-10-25T10:53:47Z

lib/core/src/server/config/style.config.js

+
+export default async function getStyleConfig(baseConfig) {
+  // null if file is not found
+  const customPostcssConfig = await findUp.sync('postcss.config.js', {


it should be just findUp(...) without the sync

🤦‍♂️ fixed

…storybook#4411 are fixed

kylemh · 2018-11-06T23:18:08Z

@ndelangen thoughts on deleting this branch PR and starting from the new API changes?

kylemh · 2018-11-19T04:05:02Z

Gonna take another stab at this in a different PR since direction has changed a bit

kylemh added 3 commits October 13, 2018 13:44

Add find-up to dependencies

b205946

Use postcss config in webpack if config file present

4dd6269

Merge branch 'master' of github.com:storybooks/storybook

92538a7

kylemh requested review from alterx, igor-dv and tmeasday as code owners October 13, 2018 20:55

kylemh changed the title ~~[Feature] - Use postcss.config.js when present~~ Use postcss.config.js when present Oct 13, 2018

kylemh and others added 2 commits October 13, 2018 16:28

Fix lockfile discrepancy

5ea6161

Merge branch 'master' into feature/use-custom-postcss-config

9ab0c20

ndelangen added maintenance User-facing maintenance tasks core labels Oct 14, 2018

igor-dv reviewed Oct 14, 2018

View reviewed changes

chadfawcett mentioned this pull request Oct 17, 2018

Support css-modules #4405

Merged

ndelangen added this to the v4.0.0 milestone Oct 17, 2018

igor-dv added the feature request label Oct 17, 2018

Merge branch 'master' of github.com:storybooks/storybook into feature…

8703d83

…/use-custom-postcss-config

kylemh added 2 commits October 25, 2018 01:31

Simplify and spread logic into separate file

3337585

Restructure to include logging statement

9250f1e

igor-dv reviewed Oct 25, 2018

View reviewed changes

kylemh added 4 commits October 25, 2018 03:26

Fix config object

6f22d66

Asyncify findUp

d413841

Temporarily commit example

d785ab3

Merge branch 'master' of github.com:storybooks/storybook into feature…

7bac401

…/use-custom-postcss-config

kylemh requested review from Hypnosphi and ndelangen as code owners October 25, 2018 10:44

kylemh requested a review from usulpro as a code owner October 25, 2018 10:44

igor-dv reviewed Oct 25, 2018

View reviewed changes

Actually use async

dfa78c3

Wurielle added a commit to Wurielle/pristine that referenced this pull request Oct 26, 2018

- Storybook on hold until storybookjs/storybook#3850 and storybookjs/…

049c680

…storybook#4411 are fixed

ndelangen modified the milestones: v4.0.0, next Oct 29, 2018

Hypnosphi changed the base branch from master to next November 5, 2018 17:04

kylemh closed this Nov 19, 2018

kylemh deleted the feature/use-custom-postcss-config branch November 19, 2018 04:05

thollander mentioned this pull request Oct 21, 2019

Core: Allow custom postcss config #8498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use postcss.config.js when present #4411

Use postcss.config.js when present #4411

kylemh commented Oct 13, 2018 •

edited

Loading

codecov bot commented Oct 14, 2018 •

edited

Loading

igor-dv Oct 14, 2018

kylemh Oct 14, 2018 •

edited

Loading

igor-dv Oct 15, 2018

kylemh Oct 15, 2018 •

edited

Loading

igor-dv left a comment

igor-dv Oct 14, 2018

kylemh Oct 14, 2018

igor-dv Oct 15, 2018

kylemh Oct 15, 2018

kylemh commented Oct 14, 2018

kylemh commented Oct 17, 2018

storybook-safe-bot commented Oct 25, 2018

igor-dv Oct 25, 2018

kylemh commented Oct 25, 2018 •

edited

Loading

igor-dv Oct 25, 2018

kylemh Oct 25, 2018

kylemh commented Nov 6, 2018

kylemh commented Nov 19, 2018

Use postcss.config.js when present #4411

Use postcss.config.js when present #4411

Conversation

kylemh commented Oct 13, 2018 • edited Loading

What I did

How to test

codecov bot commented Oct 14, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

kylemh Oct 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylemh Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

igor-dv left a 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

Choose a reason for hiding this comment

kylemh commented Oct 14, 2018

kylemh commented Oct 17, 2018

storybook-safe-bot commented Oct 25, 2018

Choose a reason for hiding this comment

kylemh commented Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylemh commented Nov 6, 2018

kylemh commented Nov 19, 2018

kylemh commented Oct 13, 2018 •

edited

Loading

codecov bot commented Oct 14, 2018 •

edited

Loading

kylemh Oct 14, 2018 •

edited

Loading

kylemh Oct 15, 2018 •

edited

Loading

kylemh commented Oct 25, 2018 •

edited

Loading