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: add name flag #1757

Merged
merged 4 commits into from Aug 21, 2020
Merged

feat: add name flag #1757

merged 4 commits into from Aug 21, 2020

Conversation

anshumanv
Copy link
Member

What kind of change does this PR introduce?
feat

Did you add tests for your changes?
Yes

If relevant, did you update the documentation?
Yes

Summary

Add name flag.

Does this PR introduce a breaking change?
No

Other information

@anshumanv anshumanv requested a review from a team as a code owner August 18, 2020 19:32
Copy link
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

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

Looks good. Just one doubt.
How name assignment would work in case of multiple configurations through the flag you created?

@anshumanv
Copy link
Member Author

How name assignment would work in case of multiple configurations through the flag you created?

yeah we should have a test with multiple config

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.

Do you want to backport is for webpack@4? I think core flags have the name flag

@anshumanv
Copy link
Member Author

Yes this is for v4, https://v4.webpack.js.org/configuration/other-options/#name

v5 already supports this via core flags @evilebottnawi

@alexander-akait
Copy link
Member

Looks good. Just one doubt.
How name assignment would work in case of multiple configurations through the flag you created?

Interesting case, ideas?

@anshumanv
Copy link
Member Author

Interesting case, ideas?

Default behaviour right now is to apply it to all config, not sure if that's right for name

@anshumanv
Copy link
Member Author

Devtool flag applies to all the configs

@alexander-akait
Copy link
Member

This options make sense only with single --config-name

@anshumanv
Copy link
Member Author

@snitin315 how does the core flag works in case of multiple config?

@snitin315
Copy link
Member

@snitin315 how does the core flag works in case of multiple configs?

webpack --name=web purpose is just to set compiler.options.name = 'web' (which sets compiler.name = web) for a single webpack config file just like other cli-flags (for example --mode).

Name of the configuration. Used when loading multiple configurations.

This property options.name is useful in case if we export an array of multiple configs to the webpack compiler. It shows us output stats with name of the child compiler, so we can easily differentiate the stats as per configs -

module.exports = [{
     name: 'first'
  }, 
  {
     name: 'second'
  }, 
  {
     name: 'third'
}]

Screenshot at 2020-08-20 07-09-31

In case if you don't provide options.name in config the output will be like -

Screenshot at 2020-08-20 07-19-36

@snitin315
Copy link
Member

snitin315 commented Aug 20, 2020

But we don't support setting options for multiple configurations. Hence --name doesn't do anything in case multiple configs are exported as an array.
Screenshot at 2020-08-20 07-30-28

@alexander-akait
Copy link
Member

I think we need discussion about multiple configurations and flags, what is the logic now?

@snitin315
Copy link
Member

snitin315 commented Aug 20, 2020

I think right now we support -

  • choosing a config file from multiple config files via --config.
  • falling back to a default config based on priority in case of multiple config files.
  • choosing a config via --config-name if an array of configs is exported. If we don't provide --config-name all the configurations will run.

@alexander-akait
Copy link
Member

Here interesting case, some options can't be overridden when you have multiple configurations, for example --name potential break build. So i think, we need list of this options and throw an error about this. I think same for UniqueName https://github.com/webpack/webpack/blob/master/schemas/WebpackOptions.json#L3165

@snitin315
Copy link
Member

+1

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.

Let's open an issue about forbidden flags for multiple compilations and implement it in the other PR

alias: "i",
description: "Use webpack interactively",
group: BASIC_GROUP
} */
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 we should focus on this after release too

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.

Good job

@alexander-akait
Copy link
Member

/cc @webpack/cli-team need review

Copy link
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@alexander-akait alexander-akait merged commit ad0779f into webpack:next Aug 21, 2020
@anshumanv anshumanv deleted the name-flag branch August 21, 2020 11:51
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

5 participants