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

feat: added config command #4126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

samrudh3125
Copy link

What kind of change does this PR introduce?

It introduces a new feature which is the config command which has three options i.e file, name and include.

Did you add tests for your changes?

I have added tests for name and file option but for include didn't add tests as at this stage it just prints the present configuration in the console.

Summary

The issue #3537 is solved in this PR. This was a very good issue to work on as i could understand the complete codebase of webpack as I am aiming at working in one of the projects of webpack in GSOC.

Does this PR introduce a breaking change?

Yes there are a lot of changes as I have completely deleted the built in commands --config and --config-name and introduced a whole new command in their place.
Other information

@samrudh3125 samrudh3125 requested a review from a team as a code owner March 23, 2024 08:28
Copy link

linux-foundation-easycla bot commented Mar 23, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

Yes there are a lot of changes as I have completely deleted the built in commands --config and --config-name and introduced a whole new command in their place.

This is not the intend of the issue, what we want to achieve is a new --print-config/--show-config cli flag similar to other tools like jest which will output the whole webpack config on console.

@samrudh3125
Copy link
Author

Yes but in the issue itself you had asked to create a webpack config command which has 3 options:
1 file- which replaces --config
2 name- which replaces --config-name
3 include- which is the additional options which prints the whole configuration in the console.

@webpack-bot
Copy link

Hi @samrudh3125.

Just a little hint from a friendly bot about the best practice when submitting pull requests:

Don't submit pull request from your own master branch. It's recommended to create a feature branch for the PR.

You don't have to change it for this PR, just make sure to follow this hint the next time you submit a PR.

@snitin315
Copy link
Member

Yes but in the issue itself you had asked to create a webpack config command which has 3 options: 1 file- which replaces --config 2 name- which replaces --config-name 3 include- which is the additional options which prints the whole configuration in the console.

No this is not the intent of the issue. Those are options for this command to print selective configurations and can be added later.

@samrudh3125
Copy link
Author

Then can you please elaborate the issue once again so that I change it accordingly

package.json Outdated Show resolved Hide resolved
}

const compiler = webpack(config.options as webpack.Configuration[]);
compiler.run((err, stats) => {
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to run this

Copy link
Author

Choose a reason for hiding this comment

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

No it's running only in case of --file and --name for --inckude I will remove it

}

if (options.include) {
const includeOptions = webpack.config.getNormalizedWebpackOptions({});
Copy link
Member

Choose a reason for hiding this comment

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

Also I think we need to apply defaults, so you will see how webpack sees your configuration

Copy link
Author

Choose a reason for hiding this comment

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

Actually we need to comment the default values beside them which is getting very complex so shall I apply the defaults?

Copy link
Member

Choose a reason for hiding this comment

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

You can get default value applyWebpackOptionsBaseDefaults and applyWebpackOptionsDefaults from:

const {
	applyWebpackOptionsDefaults,
	applyWebpackOptionsBaseDefaults
} = require("webpack/config/defaults");

How I see this logic, you load config file/files, normalize and them apply default.

Also we have two possible commands:

  • webpack config just print full resolved configuration, i.e. after apply normalization and default
  • webpack config --diff show only options which are different after normalize and default, so you can undestand which options you don't need because they are default

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.

Also please don't remove tests

@samrudh3125
Copy link
Author

My bad actually since I removed the command I deleted the tests

@webpack-bot
Copy link

@samrudh3125 Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@alexander-akait Please review the new changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants