Skip to content

feat(@angular-devkit/build-angular): add maximumInlineSize and excludeFromInlining options to browser build #11826

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

Closed
wants to merge 1 commit into from

Conversation

alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented Aug 9, 2018

Add maximumInlineSize and excludeFromInlining options to browser build

This 2 options add the flexability to the consumers to choose what to exlcude from being inlined in the final css output and also the maximum size of the resouce in KiB that can be inlined.

…ludeFromInlining` options to browser build

This 2 options add the flexability to the consumers to choose what to exlcude from being inlined in the final css output and also the maximum size of the resouce in `KiB` that can be inlined.
@filipesilva filipesilva added the needs: discussion On the agenda for team meeting to determine next steps label Aug 16, 2018
@alexeagle
Copy link
Contributor

alexeagle commented Aug 16, 2018

We're reaching out to chrome team to help us understand when is the right time to inline fonts and images.
I'd rather not add configuration options if it means users need to discover and use these options or else they get the wrong behavior aka. footgun

@ngbot
Copy link

ngbot bot commented Aug 23, 2018

Hi @alan-agius4! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@alexeagle
Copy link
Contributor

Comment from @addyosmani

Thanks for reaching out about when to inline resources. It was unclear from the thread whether you're distinguishing how you handle different types (web fonts, images etc).

We typically don't see a large number of sites inlining web fonts. Given 60% of origins load at least one web font and occasionally it's multiple families, I'm not sure that we should encourage breaking caching by inlining fonts.

A more popular pattern is preloading your fonts or images if they're critical to early paints to ensure the browser fetches them earlier but without breaking caching. Inlining images (e.g. base-64ing) can have negative perf connotations on mobile devices (last I heard).

Supporting easier inlining seems okay as long as it's an opt-in config authors use themselves and there's documentation warning them about the caveats.

And from @KenjiBaheux:

Inlining:

  • is bad for repeat visits
  • is bad for performance (e.g. delays parsing of the rest of the resource where the inlining is happening)
  • can lead to performance footguns (i.e. forgotten resources that are no longer needed, or inlining the whole family when only one face is needed / critical)
  • makes font-display pointless (the font has been loaded regardless in all cases)
  • bloats the bytes count
  • seems like it would be fast for the UA to consume but it actually isn't the case (still has round trips and main thread business interruptions happening <= I think we tried to fix some of that but unclear if we succeeded, in part we also don't want to encourage inlining).

My take on this is that we shouldn't do inlining at all unless we have a way to guide users towards the right config, or possibly investigate how to do preloading of critical assets instead.

@alan-agius4 alan-agius4 deleted the feat_inline_resources branch August 28, 2018 18:06
@hansl hansl removed needs: discussion On the agenda for team meeting to determine next steps labels Jan 24, 2019
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants