fix(build): Use static files for styles

[jimitndiaye]

Uses extract-loader to prepare static files for styles rather than use style-loader and thus avoids FOUC.

Fix#2148
Fix#2020
Fix#2826

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[googlebot]

CLAs look good, thanks!

[filipesilva]

@TheLarkInn can you review?

[filipesilva]

@jimitndiaye had a look with @TheLarkInn, it looks good but for this sort of enhancement we do need some tests.

The ones in https://github.com/angular/angular-cli/tree/master/tests/e2e/tests/build/styles can already validate that the dev build works as before, but we need to know that the prod builds work as intended.

Getting the styles file with hash is kinda hard since the hash is random, my suggestion is to both check index.html and match the filename the same way as in https://github.com/angular/angular-cli/blob/master/tests/e2e/tests/build/prod-build.ts#L36

Prod tests for css/sass/less would be ideal.

[jimitndiaye]

@filipesilva Sure, I'll add a check in https://github.com/angular/angular-cli/blob/master/tests/e2e/tests/build/prod-build.ts#L36 to check for the existence of the hashed styles file in index.html.
For the css/less/sass tests however, do you have any objections to adding them to their respective test file under https://github.com/angular/angular-cli/blob/master/tests/e2e/tests/build/styles? This would obviously add an additional ng build --prod to the promise chain plus equivalent asserts.

[jimitndiaye]

Ended up modifying https://github.com/angular/angular-cli/blob/master/tests/e2e/tests/build/styles/css.ts to include additional assertions for the prod build, since it was the only test case that dealt with the styles bundle.

[jimitndiaye]

e2e tests now include

• verifying that prod build adds styles.*.bundle.css to index.html where * is a content hash.
• verifying that css bundling works for css, less and sass, merging all styles into a single minified (for prod) CSS bundle.

[jimitndiaye]

@filipesilva The extract-text-plugin removes all the css from styles.bundle.js and puts in styles.bundle.css, leaving the javascript file largely empty (just no-op functions remain). This JS file is till being included in index.html, even though it doesn't really serve a useful purpose any more. What do you think about excluding it from the HtmlWebpackPlugin. At the moment it is just an extra HTTP request that serves no useful purpose. Just a few bytes but still unnecessary.
An alternative is to list the input styles in the main entry, avoiding the creation of styles.bundle.js while still creating styles.bundle.css

[clydin]

There is an open webpack issue regarding the no-op file: webpack/webpack#1967

[filipesilva]

Thanks for adding the tests @jimitndiaye, they look good! You're right that the css.ts file is the only one that really tests the global styles bundle. Tbh it should be called global-styles.ts instead I think... That was an oversight of the test refactor we had some time ago.

Regarding the no-op file, did you test removing it via HtmlWebpackPlugin? As I understand from @clydin's link, it still has a module, and I suppose it's still imported. If it's removed, it should just break the module loading I think. So in that sense it's better to wait for it to be fixed in webpack.

[filipesilva]

I marked it for a merge, unless you have more changes you want to do.

[jimitndiaye]

No if we're ok with importing the no-op file then i think it's all good

[jimitndiaye]

There's the option of including the global styles in the main entry though. This would mean styles.bundle.js would not be created and styles.bundle.css would be extracted from the main entry

[filipesilva]

That's a possibility, but would prefer it to be in a separate PR so that it could be evaluated separately as a workaround.

[jimitndiaye]

Sure. I'll get on that once this one is merged

[clydin]

With the lack of movement on the web pack issue that might be a better option. Although, personally I like the separation; seems cleaner.

[filipesilva]

Thanks for the great on this @jimitndiaye!

[hansl]

You never resolve this promise, causing tests to stop early. This test is causing our e2e to not continue.

Also, no need to return a promise, just return the glob.sync.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}