-
-
Notifications
You must be signed in to change notification settings - Fork 603
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
fix: warn about merge config resolution cases #1674
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I think we should discuss about configuration files. We have webpack.config.development.js
, we have webpack.base.js
, we have configurations from .webpack
directory. It can be very misleading for developers.
We should provide clear and logical hierarchy structure.
Maybe we should not fallback to webpack.base.js
here. Just throw an error what we don't find the configuration, because it can be misleading for developers too
Agree, but .base is usually used to keep shared config for prod and dev env, which is eventually merged with the respective config hence this fallback was added in past I think, we're already notifying user in case we fallback, what do you think? @evilebottnawi |
@anshumanv Do we have this logic for v3? |
No, we don't. Should we remove fallback? |
I think we shouldn't fallback as well, Developer never intends to use |
@anshumanv Let's discuss about it. What configuration files do we have and how are they inherited and them priorities? Maybe we can do simple drawing/picture? |
Using as standalone is out of the picture |
Yep we can, I'll come up with it in a while 👍 |
[
'webpack.config',
'webpack.config.dev',
'webpack.config.development',
'webpack.config.prod',
'webpack.config.production',
'.webpack/webpack.config',
'.webpack/webpack.config.none',
'.webpack/webpack.config.dev',
'.webpack/webpack.config.development',
'.webpack/webpack.config.prod',
'.webpack/webpack.config.production',
'.webpack/webpackfile',
] @evilebottnawi this the order of priority of files, in increasing order of priority. eg if We do not resolve for |
Also, should we document it somewhere for future reference, it's present in code only for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change logic for this, it is the interesting case, when I use
webpack --config' './1.js', '--config' './2.js
I would think that we want to run both configurations, like multi compiler mode.
But, if i write
webpack --merge --config' './1.js', '--config' './2.js
I want to merge configurations.
I think this logic is simpler and clearer.
If configuration doesn't exists in --config
we should always throw an error.
Also provide --config
options disable auto resolving configurations, because I have clearly indicated that I want to use.
And yes we should documented configurations and their priority
@evilebottnawi that makes sense, there are a several steps involved to achieve that, let's do them bit by bit. I'll start by removing the fallback to base config from here, just the error message that the merge config is not found should be good for now. |
@evilebottnawi are there any cases when the user only wants to merge to one config and use another as is? Eg - In this case the user only intends to merge base config with |
I don't think we should support is, because it is misleading, if you want to more three configuration, you can use
If you want to run 3 configuration:
If you want to merge first and second configurations, use code for this, no need handle very complex logic |
Seems fair @evilebottnawi , I've dropped the support for fallback to default config in merge here, will add support for multi config after this. 👍 |
expect(stats.isFile()).toBe(true); | ||
done(); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's override the other option, for example filename
, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added 1fdf2ae 👍
What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
NA
Summary
Does this PR introduce a breaking change?
No
Other information