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(defaults): default entry, output, mode #6140

Merged
merged 10 commits into from Jan 3, 2018

Conversation

TheLarkInn
Copy link
Member

@TheLarkInn TheLarkInn commented Dec 15, 2017

What kind of change does this PR introduce?
This defaults the following properties to their following values:

entry: "./src";
output <== turns out this was defaulted but just removed the webpack-cli validations (separate branch)
mode: "production" <== no reason why this shouldn't be the case

Did you add tests for your changes?
Not sure if needed w/ changing defaults besides schema

If relevant, link to documentation update:
Will add if accepted.

Summary
#0CJS

Does this PR introduce a breaking change?

Other information
This PR depends on webpack/webpack-cli#217 merging so that the validation from the CLI doesn't fail runs of webpack. So likely I'll need to update this and add the peerDep

@addyosmani
Copy link

mode: "production" <== no reason why this shouldn't be the case

Strongly supportive of this default :)

@TheLarkInn TheLarkInn closed this Dec 15, 2017
@TheLarkInn TheLarkInn reopened this Dec 15, 2017
@TheLarkInn
Copy link
Member Author

Some notes on this @sokra
Failure 1: My guess is that we should be running mode: development for the caching tests?

@TheLarkInn
Copy link
Member Author

TheLarkInn commented Dec 15, 2017

//TODO:

  • Failure 1. Update caching tests to set default mode to validate caching and speed
  • Failure 2-5. Remove tests that validate an empty configuration now that webpack can build without one

@TheLarkInn TheLarkInn self-assigned this Dec 15, 2017
@azz
Copy link

azz commented Dec 16, 2017

What about respecting package.json "module" for entry, "main" for output?

@TheLarkInn
Copy link
Member Author

@azz that is really only a convention meant for library packaging, and not specific for Web Application. Overall, any of these properties are easily to override.

@TheLarkInn
Copy link
Member Author

Also, don't merge this until we remove the validations from the webpack-cli. I will PR this right now.

@TheLarkInn
Copy link
Member Author

@sokra I was not seeing these test failures locally. Any advice welcomed. The rest are cleaned up though.

@EugeneHlushko
Copy link
Member

hi @TheLarkInn
wanted to add a commit but no perms, set options.optimization.minimize = false;
here: ./test/Compiler-caching.test.js for compiler caching to work

@jsf-clabot
Copy link

jsf-clabot commented Dec 18, 2017

CLA assistant check
All committers have signed the CLA.

@TheLarkInn
Copy link
Member Author

@EugeneHlushko I'll give it a shot. I did have previously set mode: to development to avoid this, but we shall see.

@TheLarkInn
Copy link
Member Author

Ah crap I pushed code from new dev build. When I find a chance I'll remove "root" commit and repush, otherwise @sokra feel free to also.

@sokra
Copy link
Member

sokra commented Dec 18, 2017

mode: "production" <== no reason why this shouldn't be the case

It was intended to not be the default.

The idea was to emit a warning be default that reminds you to create two configuration (dev vs prod) and forces you to actively set the mode option. It basically brings the mode option into the developers mind. This way you can't write a config without thinking about "is this development or production": If you omit the mode option you get a warning. If you set the mode option to development or production in your config you'll see it in the configuration that there must be something wrong. It forces you to create two configs for dev and prod (or one config with env switch).

If mode: "production" would be default, it would be (in my opinion) a more difficult getting started experience. Assuming a newbe doesn't read the documentation, (s)he won't know about the mode option. Builds would be slower and no devtools would help her/him. We should yell at her/him to use the mode option and think about how to configure webpack for production vs development. It basically forces a newbe to add one extra line to the config mode: "development" at the start and makes her/him aware that this config is only suitable for development and (s)he should think about configuration when going to production...

@sokra
Copy link
Member

sokra commented Dec 18, 2017

Regarding the entry and output.filename:

I also tried this a while ago, but you get problems with these defaults when using the CLI.

webpack allows this CLI syntax: webpack <entry[]> <output>. entry and output must be configured either in config or on command line.

With entry and output having default values this will get unclear:

webpack a b could mean:
entry: ["a", "b"], output: "[name].js" or entry: "a", output: "b"

webpack a could mean:
entry: ["a"], output: "[name].js" or entry: "./src", output: "a"

webpack could mean:
entry: "./src", output: "[name].js or you are running webpack not in the directory where your config file lives.

@michael-ciniawsky
Copy link
Member

I agree with @sokra here, mode shouldn't be set by default and instead users should be clearly 'nudged' to use mode instead. If mode would be set it needs to default to development, since normally a new project actually needs to be developed first, running in production mode right from the start is confusing imho. entry is something that's too specific to the particular project itself to be really defaultable. output set to e.g filename: [name].(bundle)*.js (development) && filename: [name].[chunkhash].(bundle)*.js (production) alongside with chunkFilename: [name].(chunk|async)*.js (development) && [name].[chunkhash].(chunk|async)*.js (production) would be a good default on the other hand :)

* suggestion but maybe not needed at all (and too verbose)

@TheLarkInn
Copy link
Member Author

@sokra we should find a way to work around the cli issues, in regards to mode, I think it makes sense to give users a production bundling experience out of the box. At the least we can guarentee that for the 20-30 percent who don't understand, get performance or anything else, that we are ensuring at least code is minifies etc.

@TheLarkInn TheLarkInn force-pushed the feature/add-entry-default-value branch from f78d650 to 5e8700f Compare December 18, 2017 20:18
@TheLarkInn
Copy link
Member Author

Okay lets split hairs here:

  1. For the CLI issues: Let's remove some CLI features. This is for v4 so its the right time to add the breaking change. I'd say if we need to break the array usage we could, or have a more explicit way to support it.

  2. Performance being on by default:

If mode: "production" would be default, it would be (in my opinion) a more difficult getting started experience. Assuming a newbe doesn't read the documentation, (s)he won't know about the mode option. Builds would be slower and no devtools would help her/him.

This is actually a really great point. I think we can step it up another notch instead of just forcing them to specify, we could still have it enabled to production + but then also warn them and say:

"Configuration option mode has not been specified and will default to 'production'. For more information, see https://webpack.js.org/configuration/mode"

@sokra
Copy link
Member

sokra commented Dec 18, 2017

I actually thought about making mode a required property via schema in webpack 5.

@TheLarkInn
Copy link
Member Author

webpack/webpack-cli#227 Dependent issue up for @ev1stensberg to review.

@evenstensberg
Copy link
Member

@TheLarkInn @michael-ciniawsky left a review

@TheLarkInn
Copy link
Member Author

These errors make sense but I don't have time at the moment to fix them. Hopefully this week (maybe), next for sure.

@sokra
Copy link
Member

sokra commented Dec 26, 2017

I'll remove the BinTestCases in a separate PR since they have been move to webpack-cli.

@TheLarkInn
Copy link
Member Author

Alright sounds good.

@wesmdemos
Copy link

Perhaps make the warning explicit about when the default is problematic?

If mode: development is the default:

WARNING: Mode not specified, applying default 'development' optimizations. This is not intended for production. For more, https://webpack.js.org/configuration/mode

If mode: production is the default:

WARNING: Mode not specified, applying default 'production' optimizations. This is not intended for development. For more, https://webpack.js.org/configuration/mode

Separately, what about defaulting to "none" for v4 with a warning and then defaulting to development or production in v5? That would give people time to adjust. And the breaking change wouldn't come until v5. Also, with analytics you could incorporate how people use this feature to pick a default in v5.

Perhaps in v4:

WARNING: Mode not specified. Mode was introduced in v4 to simplify configuring common development and production optimizations. In v5 mode will default to 'production/development'. For more, https://webpack.js.org/configuration/mode

@TheLarkInn
Copy link
Member Author

Separately, what about defaulting to "none" for v4 with a warning and then defaulting to development or production in v5?

Since these are just defaults we are not in risk of overriding or breaking existing behaviors.

Or if we do, it would be justified in a breaking change from v3-4.

With that being said, we will still provide a warning stating that the mode was not specified.

@sokra sokra added this to the webpack 4 milestone Dec 28, 2017
@TheLarkInn
Copy link
Member Author

Hmm @sokra is there a more recommended way to push the warning if defaulted to production (and not set)?

@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@sokra sokra merged commit 048121a into next Jan 3, 2018
@sokra sokra deleted the feature/add-entry-default-value branch January 3, 2018 20:03
@sokra
Copy link
Member

sokra commented Jan 3, 2018

Thanks

@TheLarkInn
Copy link
Member Author

No prob :-D 🎉

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