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

[Proposal] Consider composing webpack config #2449

Closed
viankakrisna opened this issue Jun 1, 2017 · 6 comments
Closed

[Proposal] Consider composing webpack config #2449

viankakrisna opened this issue Jun 1, 2017 · 6 comments

Comments

@viankakrisna
Copy link
Contributor

viankakrisna commented Jun 1, 2017

This is a continuation of this comment #1651 (comment) and maybe related to #670 (but for internal use only) and I want to see a discussion about this implementation.

For a past few months I've been working on PRs and watching this repo, and a lot of the PR and issues are about adding something to the webpack config. I thought to myself, how can we add something to it without making it more complex?

And then I read this articles reading https://medium.com/making-internets/webpack-configuration-done-right-86c325a6927f ,

I've been thinking maybe we should have an INTERNAL hook/middleware for composing webpack config.

Right now AFAIK we have 3 three separate PR working on adding something to webpack config, 2 of them is optional (DLL and commonsChunk) and one is CSS module (which the user can opt it out if they use CSS in JS solution).

And for the merged ones, SWPrecache is an optional feature but we still have the webpack plugins installed, even when the user is not using it.

I think we could go with something like

const compose = require('promise-compose');
const webpackAutoDllCompiler = options => config => new Promise(resolve =>
 if (dllSrcExists()){ // only modify the config if `src/index.dll.js` exists
  config.plugins = config.plugins.concat([new webpackDllCompiler(options)]) // simplified for example purpose
  //compile it
  webpack().run(() => resolve(config))
 }
 //don't waste resource on optional feature
 resolve(config)
)
const configComposer = compose(
  webpackAutoDllCompiler({
    dllConfig,
    paths,
  }), //config resolver for dll feature
  webpackPWAConfigResolver(), //config resolver for anything related to PWA feature
  webpackCSSModuleConfigResolver(), //config resolver for CSS Module
);


//somewhere in start.js
const config = require('barebone/config/without/additional/features')
webpack(configComposer(config))

The config resolvers could be placed in react-dev-utils or config/resolvers, so additional features are neatly placed in their own modules and called when needed.

I know maybe this would make the codebase harder to follow because the configuration is hidden in the resolvers, but it would make CRA more modular. if the ejected users wanted more config / disabling a feature, they can comment out the call in the configComposer and do the configuration themselves. This way trying out new features is just a matter of adding a new line in the compose function, and no resource wasted for unused features. And because it's internal, we can control how the resolver plays with each other (plugin ordering issue, the shape of loaders section config etc). Thoughts?

@alex-pex
Copy link

@gaearon
Copy link
Contributor

gaearon commented Jun 22, 2017

if the ejected users wanted more config / disabling a feature, they can comment out the call in the configComposer and do the configuration themselves

I don't think this is an acceptable tradeoff. It's hard enough to deal with Webpack configuration, and I feel like obscuring it behind a call will make it even harder for people who eject. For many users (especially new to JS), "call this function and inline the result" when they need to change something is pretty much insurmountable.

@gaearon gaearon closed this as completed Jun 22, 2017
@Anahkiasen
Copy link

What about using something like webpack-blocks? The configuration would be both easy to understand, easily modifiable and easy to extend with custom blocks that come with CRA.

I know it's been suggested before but I can't help but to suggest it again as I really think this would be the ideal solution.

@gaearon
Copy link
Contributor

gaearon commented Jun 22, 2017

I think we should really stick to plain objects here. Otherwise we're breaking tons of existing knowledge (e.g. all StackOverflow answers, Webpack docs, library READMEs etc). People eject because they want to escape an opinionated solution. We shouldn't lock them into another one.

@viankakrisna
Copy link
Contributor Author

viankakrisna commented Jun 22, 2017

What I get from @gaearon comment is that 'easy' is relative here. I think the contract of eject script is to give user low-level granular control of webpack itself. If we abstract it away, the ejected user still feel that they don't have full control of what is happening under the hood.

Abstracting webpack API would mean that the user needs to know how the abstraction works. It would hurt webpack-literate because there will be another doc to learn.

@gaearon
Copy link
Contributor

gaearon commented Jun 22, 2017

Yes, pretty much that.

@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants