qdouble / angular-webpack-starter

A complete Angular 6 and Webpack 4 starter seed with minimal and full featured branches. Full featured branch includes: Material Design 2 (Bootstrap 4 branch available as well), @ngrx, HMR, DLLs and optional use of Universal for server-side rendering - Supports AOT (offline) compilation, sync and lazy loading. Karma/Protractor for e2e/unit tests.
MIT License
882 stars 181 forks source link

Proposed Changes #215

Closed colinskow closed 7 years ago

colinskow commented 7 years ago

This starter strikes the perfect balance between simplicity and having most of the features I need.

I am going to be making some changes for my own project and wanted to know which ones you would like to receive pull requests for.

1) Use sass-loader to build .scss and extract-text-webpack-plugin to export the global styles. 2) HtmlWebpackPlugin used for all builds, including production (instead of copy) 3) Common chunks plugin on production builds to separate vendor files and improve load speed 4) Chunk hashes in file names for better browser caching 5) hmr-loader and all HMR code excluded from production builds 6) AOT compilation done through @ngtools/webpack instead of command line 7) Electron branch which builds exclusively for Electron applications, including e2e using Spectron

Is there a particular reason you are running command-line SASS instead of sass-loader through Webpack? I like to keep build pollution out of my source directories as much as possible.

Let me know what you want, or if you have good reasons for the way you chose to build it.

bmayen commented 7 years ago

Is there a particular reason you are running command-line SASS instead of sass-loader through Webpack?

I believe this is to support AOT which doesn't work with the require() approach

DzmitryShylovich commented 7 years ago

Is there a particular reason you are running command-line SASS instead of sass-loader

it's not necessary any longer. you can use aot webpak plugin. this repo will migrate very soon

qdouble commented 7 years ago

2) Is due to universal compatibility, but I may change that on the other branches when I make the next update 3) Separating the files also produces larger bundles as treeshaking is more effective when everything is in one 5) Not understanding what you mean here 6) We will be switching to ngtools soon, there were a lot of issues with it that I wanted to be fixed before migrating...but I feel it's safe to migrate now. 7) I don't use that, so it'd be difficult for me to maintain that

As mentioned above, the command line sass had to to do with AOT compatibility, but should not be needed when we switch to using ngtools

bmayen commented 7 years ago

@qdouble, would you consider using webpack-merge to split config between common, dev, prod, and test environments and also allow for customization this way instead of via constants file?

qdouble commented 7 years ago

@bmayen for me it seems easier to maintain a single config file then going back and forward between different files.

bmayen commented 7 years ago

IMO, it's less about the difficulty of going back and forth between files than it is about being able to define a single common config for different environments and easily overriding any of it.

The constants.js approach is rather proprietary. We have a large team with many projects; using this setup requires everybody to not only understand webpack config, but also a proprietary way to customize it.

Last but not least, if we want to add different config for more environments than you've defined here, it's not very straightforward/clean.

What you gain in simplicity of having a single config, you lose in clarity and flexibility. I think the latter is far more beneficial to teams consuming this repo.

qdouble commented 7 years ago

Well I don't think the constants.js file is really related to the issue of having separate configs at all... that's there simply to define custom project settings without having to touch the main config. There are indeed tradeoffs for having separate config files, one of which being that can create a bigger maintenance burden and can be harder to reason about. I suppose I could accept a PR that separates config files if done cleanly enough, but it isn't particularly priority for me.

bmayen commented 7 years ago

Well I don't think the constants.js file is really related to the issue of having separate configs at all

In the sense that using webpack-merge enables both custom settings and separate/additional environment configs. Two birds with one stone.

Just curious, do you see more of a downside to not taking this approach? Feature-wise this repo is the most forward-thinking of the options out there, but the implementation makes it difficult to integrate into a larger team.

qdouble commented 7 years ago

@bmayen this repo is largely based off what I find convenient to use in my own projects... I'm able to use this starter configuration in different projects without any particular need for separate config files. As I said, if someone wants to go ahead and submit a PR for the different branches that creates separate configs and it's done cleanly enough, I'd consider merging it...but as I don't have a particular need for that, I don't plan on taking time to rewrite it.

colinskow commented 7 years ago

Personally I find multiple configuration files less DRY and harder to figure out. I like your approach here @qdouble.

bmayen commented 7 years ago

How would it be less DRY? It wouldn't be repeating the entire config for each environment, it would be pulling the common config out into its own file, allowing individual environments to only define their unique parts.

colinskow commented 7 years ago

The advantage to separating out vendor chunks is that the vendor files typically change less than the application and are most of the size. So when the main application code changes the users only need to re-download a small portion of the application.

If the chunk hashes are added to the filename it makes browser caching much more efficient.

I submitted a PR with dependencies upgraded as far as they could go without breaking. Next step is to switch to @ngtools/webpack AOT compilation.

colinskow commented 7 years ago

@bmayen using webpack-merge it is awkward, for example, if I need to insert one loader at the beginning and one at the end and get plugins in a certain order.

I find it much order to compose the loaders with if-then statements based on the environment with one file. Unless the config is really, really complex I find it easiest to figure out what is going on by reading through one file, rather than 10+ files that other starter projects use.

bmayen commented 7 years ago

For argument's sake, what would you do if you used this config as a base for, say, 10 different projects with different devs on each, and you have a few additional environments for each and some other custom config you want to add? Sure, you could make all of your changes to this config inline, but then what happens when this repo is updated with something cool like @ngtools/webpack and you want to pull those changes into all of those projects?

I understand that that kind of support is not the goal of this repo, but I think it would be unfortunate to exclude that based on not wanting to read through separate configs. Also, to be clear, I'm not advocating 10+ config files. I haven't seen an approach like that and I agree it sounds unnecessarily complex. But a common config plus dev, prod, and test doesn't sound overly complicated to me. In fact, this starter already has a separate config for tests.

colinskow commented 7 years ago

@bmayen angular2-webpack-starter may be closer to what you are looking for. I simply chose this one because it is a little more cutting edge, simpler, and I find it easier to customize to my specific build flow. Plus the author is very responsive.

qdouble commented 7 years ago

@bmayen simplicity of a repo is a valid concern and imo, having separate configs simply makes the repo more complicated than one config. While I'll admit, there are use cases in which having multiple configurations can be more beneficial there is no particular strong need for it.

First of all, once you actually go to the effort of messing with any of the configs, you've taken on the tasks of editing the webpack configuration beyond just using the constants for plug and play. It seems to me a bit extra tedious to manage and keep track of an entirely different configuration when I can just manage what I want to do with an if statement.

Now, being that we are typescript and angular users, we're all used to modularity and splitting things up into components that are a simple as possible... so I get your stance on it... but in the particular arena my project configuration, it just feels more intuitive to put the settings in one file vs navigating 5-6 different configurations.

colinskow commented 7 years ago

I realized that AngularClass/angular2-webpack-starter was closer to what I wanted, and I have already implemented most of these features there.

One thing you may consider @qdouble is to fork that repo and add NGRX & Universal. I've already added many of the most valuable features from this repo to there.

Feel free to close this issue if you would like.

qdouble commented 7 years ago

@colinskow I don't really see the point of creating carbon copies of other repos, but I'm sure we all cherry-pick certain things. I've added some of the stuff that you mentioned up top already and some I don't really plan to. Cheers.