Custom loader #428
Custom loader #428
Conversation
There was a problem hiding this comment.
I'm absolutely -1 on this as I don't see the use case or benefit this change adds at the moment. What use cases does this enable which aren't currently possible? Please also give some examples if such use cases exists, maybe there is a another way to support them or I simply don't understand it as a Loader as Loader Builder feels, in all honesty, a bit awkward in my current opinion...
|
/cc @ai What do you think about it? I don't think what we should ignore a feature request from |
|
fair questions. It enables creating higher-level loaders on top of postcss without having to copy/paste the postcss-loader. Right now there isn't a good way for tools to both provide postcss based transforms as well as letting downstream users customize for other things. For instance Gatsby's css pipeline defaults to autoprefixing and handling some flexbugs for CSS here. This works fine but removes the ability for a user to further configure postcss with a config file, and requires that awkward function wrapping to support it in loader options. To work around this, the Alternatively this PR allows higher level tooling to produce and use slightly modified postcss-loader with preset's and option handling that are tuned to the more convention based environment the user's code is running in. Such as configuring TailwindCSS: https://github.com/jquense/docpocalypse/blob/master/packages/theme/tools/theme-loader.js without breaking Yes this could be done a few other ways, like using multiple This could also be accomplished via some additional options in postcss-loader, (like letting I also find this API a bit MEH, but there is precedent for it in babel, and it's not intended as an API for direct (most) users of the loader, so maybe it's less important the API be pristine (YMMV!). For reference here is Gatsby's usage of the same feature from babel-loader) Also just to be clear I'm not here with an official request from Gatsby, tho I did write/maintain a bunch of their CSS stuff, and do plan on upstreaming it there. And also would benefit from it in Gatsby, as well as in other projects I maintain like |
| const semver = require('semver') | ||
| const loader = require('../src') | ||
|
|
||
| const incomingVersion = semver.inc(postcssPkg.version, 'minor') |
There was a problem hiding this comment.
You can take PostCSS version by postcss().version or by result.processor.version in PostCSS plugin API
| @@ -0,0 +1,248 @@ | |||
| const path = require('path') | |||
There was a problem hiding this comment.
Why do we need to keep old file?
|
It is hard to review big API changes without code and use cases. I see the problem and agree that we must provide some solutions for users with this problem. But I am not sure that it is the best way (not because I know a better way, but because I do not fully understand the problem). Can we start from a little early and create an issue, where we will describe the problem and provide more use cases? Sorry, @jquense I know that it means more paperwork for you. But I afraid to take the wrong decision, reduce project maintainability, and bring to many API changes to PostCSS users (if we will need to fix API in the future). |
|
I'm happy to do the work to describe more, but I do want to try and protect myself from unnecessary work a bit.
I think there is relatively little risk, all existing tests pass, no breaking API changes, and if something goes wrong, it's easy to revert and bump. |
|
it should also be said that i'm happy to help maintain as well if anyone wants to/trusts me with a commit bit :P |
Accepting some big PR could stopped autopilot 😅. People will create a lot of issues and force us to spend more time. |
|
that's fair and true! I'm willing to help if that happens. Again i don't mean that as a negative. I also think that's a benefit to just stealing what babel did. What would helpful to describe in addition to my use-case description here #428 (comment) |
Yes but it needs to be documented and especially new users might be inclined to use it due to not understanding it's use correctly
That is my main concern, maybe Gatsby needs this kind of customization, but why e.g not simply writing a new loader then instead of adding a 'Loader Builder' to this Loader. This also has the benefit of being able to maintain this loader freely then
I did a lot of testing around to ensure it is that way, there are simply no pressing updates needed atm and no new issues being opened in a while, it's a simple loader wrapping |
Code reuse is a good thing. Sor instance, if we found and fix a bug in Also, it will simplify PostCSS major updates. |
I'm happy to add a bit note saying most ppl won't/shouldn't use this if that helps. Also the API is fairly obtuse to use just accidentally since you need to create a new file, which i think is a positive.
That is always an option, It's not great tho. The ecosystem is built on top of this loader and having other postcss based ones means there is room for underlying behavior forking. It's harder to ensure that compatible versions of postcss are used together, or that things like AST reuse work between tools, or that things like config loading are consistently handled. This isn't a big loader but there is choices made there that affect up and downstream users. Consider that css-loader tries to reuse the AST from postcss, if others make there own loader they have to remember to do that as well, maintain the same format etc as postcss-loader. Is it huge deal if these tools made their own loaders? No, but it's also not necessary and yes the cost it borne by maintainers here, and that's a thing to consider.
You did great work! not all projects need to update constantly, it wasn't a negative, just noting that there may be room for some changes now, and that users probably won't mind/notice given how infrequently they already have to think about this loader 👍 |
Right, I'm ok with this being 'hidden' for implementers (normally more advanced folks in the tooling space) in e.g
Agreed, there might be a way to handle this as setting
function loader () {
// ...
if (loader.defaults) {
// Handle defaults
}
// ...
}
loader.defaults = false
module.exports = loader
const loader = require('./loader')
// Applied after options or config loading
// to be usable as the 'normal' loader
// but with specific defaults
loader.defaults = {
plugins: []
options: {}
...etc
}
module.exports = loader
module.exports = {
// ...
module: {
rules: [{
// ...
loader: path.join(__dirname, './custom-loader.js'),
options: {
// Used as always...
}
}]
}
};
And at some point it is the better choice imho depending a how much customization is needed, the way it is currently proposed is able to basically 'hook' into everthing, which 1.) shouldn't be needed and 2.) makes reusing the code a bit senseless as it doesn't seem to provide the needed logic in general then.
I didn't recieve it as a negative, but to explain why it turned out to be likely be this way. |
|
I will open a tracking issue once I have some more context and thought about it |
|
So essentially the use case is fine, but I want to have some gurads of how one can customize it to avoid getting issues which regularly force me to ask "show your custom loader file" or the like. Adding e.g a few default |
|
I would add if we think the scope of this API is to much it could be easily pulled back to the minimal i think is necessary for these use-cases
Without proposing a very different API that would look like
|
|
Nope, I think we are going in the right direction. I just want to increase paperwork for the first planning step. What is the problem exactly? What is the current hacky solution (with code examples)? What are the limits of the current solutions? What are the possible alternatives? What other problems we can potentially fix? Unfortunately, bureaucracy is really useful thing for the big system (I spend 5 years as a Wikipedia administrator and can be sure about it 😅). |
|
@ai I think i did that here: #428 (comment) statement of the problem, workarounds and alternatives. Is there something missing you'd want to see |
|
So, do you want an extendable You define default PostCSS plugins, but they can be overridden in user’ config? Few thoughts:
|
I don't think so, There are two main things i think need to be possible:
As an example I may want to allow users to disable Gatsby's built in autoprefixing behavior, but not arbitrarily edit my postcss config. In general I'm looking for the ability to provide higher level interfaces to users, maybe even to the extent of abstracting away Postcss as a concept they need to know. As opposed treating the user as "peer" which is mostly what happens now. |
|
Another question: do we want to put plugins at the beginning of the config? Or we will add user’s plugins to the end of the list? On the one hand, PostCSS 8 will be less important. In another hand, even in Babel (they have visitor API which we will add to PostCSS 8) plugin’s order sometime it is important. What will be the logic behind this PostCSS config merging? |
|
I don't think generic config merging is the feature needed here at least it's not the one I'm looking for for this use case. One size size wouldn't fit all the cases and don't address the use case of easily changing the config based on the current file well, amount others. Plugin ordering is a great example, some plugins would need to be first and others, like autoprefixer, last. The flexibility of a function where the tooling can provide their own logic is needed |
|
@jquense yeap, I agree that custom callback is an interesting solution here. We can add |
|
That seems ok, but how would handle accepting new loader options? You need some way of split out my-custom-loader options from the base postcss-loader options before they are validated, otherwise it'll fail the JSON schema here |
I think we can add options to the loader and then just pass it to the What options do you need? |
|
The concern is more about validate, postcss-loader whitelists the accepted options you can pass it. you can't pass additional options (like ones to do additional configuration) without it yelling at you: https://github.com/postcss/postcss-loader/pull/428/files#diff-1fdf421c05c1140f6d71444ea2b27638R74 just noticed there is a bug here, the |
|
And we can always add new options from |
|
@jquense friendly ping, do you still need this feature? |
|
I still think this is a good idea and one that i would use. At the moment i've forked postcss-loader to accomplish this. Open to different or simpler API's but I think having a public API for tooling authors to build extensions on postcss-loader would be a win. |
|
@jquense Can you provide me simple use cases for this feature? I would like to evaluate this or perhaps suggest other approaches, for example you can use |
|
@evilebottnawi #428 (comment) I think describes the use cases and #428 (comment) the important requirements. I would add again that this API has already been tested and proven out for Babel, which has mostly the same concerns and use cases for tooling authors. Happy to bikeshed simpler alternatives but also don't want to reinvent the wheel for the sake of it.
|
|
@jquense I would like to see examples of usage (minimum), based on the logic described, we will need to create such logic for almost every loader, I find it a little redundant and would like to understand the problem deeper |
|
sure, here is one example of me using it to preconfigure autoprefixer for using postcss in a chain with other preprocessing https://github.com/jquense/docpocalypse/blob/master/packages/gatsby-plugin-css/utility-postcss-loader.js ( I also use it here as well to configure tailwindcss in a specific way so that it works out of the box with the gatsby theme: https://github.com/jquense/docpocalypse/blob/master/packages/theme/tools/theme-loader.js |
|
You can achieve this using https://github.com/webpack-contrib/postcss-loader#function I still can't understand use case |
That is not quite correct.
You have to do something like this ugliness which doesn't even really work: function myThemeLoader(src) {
// duplicated from postcss-loader
const config = readPostCssConfig(this.resourcePath)
return postcssLoader.call({
...this,
query: {
postCssOptions() {
return doStuff()
}
}
}, ...args)
} |
Why you need this?
Because it was never designed for this purpose I cannot understand the purpose of this.
This is a perfectly good solution. |
|
I think that similar feature in |
|
I'd also add that I've had lots of problems trying to extend webpack loaders by calling another loader in it. There are many idiosyncracies to spreading the webpack loader context. Like how some properties are recent or nulled out later making things break subtly in hard to diagnose ways. Not to mention caching issues and the like. Overall webpack loader pipeline is not designed for that sort of composition |
|
@ai The original design did not design for such a possibility, the official introduction of this will potentially create a situation where we will have to do this for each loader, then we need to completely revise the design to avoid reimplementation this for each loader |
|
@evilebottnawi I understand that is hard. But making thing hard should not be a reason to not do it. If users need some feature, we need to help it (when we will have a time). If they want to spend their time in fixing the issue, how the fact that it isard can be a blocker for them? The good reason is that will increase maintaibility. Or that the current implementation is not the best one. |
|
i will rebase! i'm quite busy with work at the moment but will get back to this |
Notable Changes
Adds a utility for tools to customize and build their own postcss-loader's based on specific needs without disrupting the core logic of the plugin. The API was taken from babel-loader where it works well https://github.com/babel/babel-loader#customized-loader
Plan on using this in the Gatsby CSS tooling as well as other places
NOTE THAT THIS INCLUDES #425
Type
SemVer
Checklist