Implement inlining

[ishitatsuyuki]

The 1KB script is now inlined in index.html. Tested locally.

inline.js is now suppressed.

No tests added so far.

[clydin]

Inline scripts add an additional security complexity; especially in combination with CSP. This should be optional, if anything. Also, the script hash should be calculated and provided if this feature is implemented.

[ishitatsuyuki]

I see there's a concern with CSP.

However, why do you think there's the need of hashing when this feature is enabled? It's something that can preferably deleted on filesystem, so I intentionally removed the hash to reduce match complexity.

[clydin]

I'm referring to a hash of the inline script content. CSP allows whitelisting of inline scripts via a hash of the inline script. (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src)
The downside is that this makes the server configuration more complicated and error-prone.

[ishitatsuyuki]

I doubt CSP is a high blocker though. Angular itself seems tricky: angular/angular#6361

The plugin I use doesn't seem to be CSP aware, so I will probably create a PR and wait for a new release.

[clydin]

Angular requires the use of the unsafe-inline option with the style-src directive. This PR would required unsafe-inline (or whitelisting) on the script-src directive as well.

[ishitatsuyuki]

Is it our responsibility to generate the hash? It's an header, and should be generated with some task runner before the deployment.

[ishitatsuyuki]

Unwipped. Added killswitch. Please take a look!

[ishitatsuyuki]

Please take a look!

[filipesilva]

@Brocco can you review?

[ishitatsuyuki]

If this looks okay, I would rebase soon. By the way, this is a feature(flag) addition, so maybe 1.1 branch should be targeted.

[clydin]

Several comments:

• I would very much prefer this was disabled by default. The CLI should be as CSP friendly has possible by default. Especially since this introduces the need of unsafe-inline for scripts; which is one of the more dangerous.
• Squash into one commit for the feature.
• I'd recommend using script-ext-html-webpack-plugin instead. It supports inline asset suppression automatically. It also has other features which may be useful in the future.
• --inline is also quite generic for an option. Thoughts on --inline-manifest? manifest is the typical webpack name for the file in question.
• Please also remove the whitespace changes in otherwise unmodified files.

[ishitatsuyuki]

@clydin

• I think disabling it will hide it from newcomers, and maybe some people will never know this optimization. CSP isn't difficult; do it via Express middleware or some sort of custom script generating hash with a HTML parser.
• The plugin you mentioned could be useful for feature addition. However, I couldn't find any evidence that it supports script output suppression. Please point me to the code if it actually does.
• There could be other things that can be inlined in future (e.g. stylesheets), and I think there's no ambiguity in the name inline as the purpose of the switch is to make CSP easy or make the HTML itself more readable.

[clydin]

• Documentation of the option will provide newcomers knowledge of the option and its effects (which need to be provided either way). If this provided a large performance improvement, I think there would potentially be a stronger case for enabling by default. But the savings of a request for a 1KB file (that will be cached) compared to a more secure default configuration (with no required additional server configuration whitelisting or workarounds) seems to not tip the scales.
• For the asset removal, see here and here
• Removing ambiguity is a useful tool (within reason) to limit the number and severity of support issues. In this case inline could mean any number of things to someone. For Example: Could it inline the manifest? Could it inline the global stylesheets? Could it inline the entire application? In the future, the CLI could offer a configuration option on the styles field to allow certain stylesheets to be inlined. Would this command option control that as well even though it most likely would not if implemented?
  But you could make the case that it's bike shedding at this point. I defer to @Brocco / @filipesilva for the naming.

[ishitatsuyuki]

Rebased. Untested.

[ishitatsuyuki]

Review?

[hansl]

I agree with the general sentiment that the current behaviour should be the default for this new flag (false). Also, I'm not convinced that ScriptExtHtmlWebpackPlugin should be used or is needed. Isn't this just removing the CommonChunkPlugin for inline?

[filipesilva]

I share @clydin's concerns on this: there is a benefit and a demand for this functionality, but we neither should change the existing behaviour nor put the extra responsibility of extra CSP configuration on users.

It might be trivial for some users, but not so much for others, and possibly completely impractical for some server setups.

For this reason I think it should default to false.

[hansl]

@ishitatsuyuki Could you explain what problem this PR is trying to fix? There might be a better approach.

[ishitatsuyuki]

This is a new feature, not fix. The related issue is #2307.

I'm on a trip currently. Will disable it by default.

[ishitatsuyuki]

Rebased and addressed review comments.

The license check is currently failing due to a dependency update. It's not my fault, but please find a way to workaround it (LGPLv2 is not currently accepted).

[ishitatsuyuki]

I have rebased the changes and I believe I have addressed all the concerns. Can we get this into 1.3?

[GuskiS]

Can this be extended (somehow) to allow inline critical (or any) CSS?

[ishitatsuyuki]

@GuskiS no, that is server side renderer's job.

[GuskiS]

Why is that the only choice? For example, I would like to have static HTML that has inlined critical CSS, some sort of app shell (navigation and loading animation), that is gzipped and then served statically - not changed by SSR.

[ishitatsuyuki]

If you want to inline "critical" css, determine it and put it manually. A rule being used or not can only be determined after render.

This only inline webpack manifest (runtime).

[ishitatsuyuki]

Closing due to inactivity. Feel free to reuse my code.

[dominique-mueller]

Any chance for further discussion on this topic?

I find it rather weird to have a 5-line JavaScript file whichs needs to be fetched separately, especially from a performance perspective. I would at least expect the Angular CLI to have some kind of option for (actually) inlining the inline bundle into the index.html (such as this PR would introduce). Defaulting to false would also be fine.

Also, wouldn't it be possible to simplify put the inline bundle stuff into the main bundle directly? Or am I missing something here?

[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.}