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

Allow tooling authors to provide custom postcss factory through options #511

Merged
merged 1 commit into from
Jan 21, 2021

Conversation

phated
Copy link
Contributor

@phated phated commented Jan 15, 2021

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Ref #510

I know @alexander-akait said it is out-of-scope, but I needed to write the code anyway. So I figured I'd make the code suitable to upstream and submit it anyway—under the assumption this will be closed.

I'll be publishing my fork as @phated/postcss-loader for our use in Storybook until we can make the 7.0 breaking release—at which time, we'll be able to require users to rely on peerDeps and switch back to this upstream project.

Cheers! 🚀

Breaking Changes

Additional Info

@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #511 (14b9cb5) into master (d7657e5) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #511      +/-   ##
==========================================
+ Coverage   97.95%   97.96%   +0.01%     
==========================================
  Files           5        5              
  Lines         244      246       +2     
  Branches       80       81       +1     
==========================================
+ Hits          239      241       +2     
  Misses          5        5              
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)

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 d7657e5...14b9cb5. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide example of usage? Do you want to use postcss@7 or postcss@8? Why not validate it on boilerplate side? In theory I can add this as the migration options (and when we drop postcss@7 support we remove it)

@phated
Copy link
Contributor Author

phated commented Jan 18, 2021

Currently, the way a user consumes Storybook looks (more-or-less) like:

/users-project-root
├─┬ node_modules
│ └─┬ storybook
│   ├── postcss-loader
│   └── postcss@^7
└── storybook-config-file

The internally-included postcss@^7 dependency cannot be removed before the 7.0 released, and users are using it implicitly.

However, a new option can be exposed that allows users to specify a custom postcss factory that has priority over the implicit dependency using this feature. So they would do:

/users-project-root
├─┬ node_modules
│ ├─┬ storybook
│ │ ├── postcss-loader
│ │ └── postcss@^7
│ └── postcss@^8 <-- add postcss 8 as top-level dep
└── storybook-config-file <-- add require("postcss")

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's rename customPostcss to implementation, we have the same option in sass-loader (https://github.com/webpack-contrib/sass-loader/blob/master/src/options.json#L4)

@phated
Copy link
Contributor Author

phated commented Jan 19, 2021

@alexander-akait great thought! I've updated to be more consistent with sass-loader. Do you have any recommendations for getting the snapshots to succeed consistently across node versions?

test/cjs.test.js Outdated
@@ -1,3 +1,4 @@
// eslint-disable-next-line import/no-named-as-default, import/no-named-as-default-member
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is global eslint problem, we don't have this rules

src/index.js Outdated
@@ -1,7 +1,7 @@
import { getOptions } from "loader-utils";
import { validate } from "schema-utils";

import postcss from "postcss";
import ourPostcss from "postcss";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's return to postcss

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I ran into was naming the local variable postcss too. I can change that and put this back to postcss

README.md Outdated
### `implementation`

Type: `Function`
Default: `undefined`
Copy link
Member

@alexander-akait alexander-akait Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove Default from here, because we describe this below

@alexander-akait
Copy link
Member

Also can you accept CLA?

@phated
Copy link
Contributor Author

phated commented Jan 20, 2021

I think the CLA app is bugged

Screen Shot 2021-01-20 at 10 35 00 AM

@phated
Copy link
Contributor Author

phated commented Jan 20, 2021

@alexander-akait I addressed your comments and squashed the commits. I removed my package-lock updates and other things that made the tests pass locally for me, I think those should be fixed outside this feature.

@alexander-akait alexander-akait merged commit deac978 into webpack-contrib:master Jan 21, 2021
@alexander-akait
Copy link
Member

Thanks

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