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

RFC 149 - Switch to per-page asset loading #152

Merged
merged 11 commits into from
Aug 24, 2022

Conversation

injms
Copy link
Contributor

@injms injms commented Aug 2, 2022

Component stylesheets and JavaScript should only be included for components present on a page on GOV.UK. This will lead to a reduction in page size, improve the ability of the browser to cache assets - which in turn improves performance for user journeys and return visits - and help reduce our reliance on manual asset auditing.

Deadline: Tuesday 16 August 2022

Rendered version

@injms injms marked this pull request as ready for review August 2, 2022 16:12
@injms injms changed the title Add RFC 149 - Switch to per-page asset loading RFC 149 - Switch to per-page asset loading Aug 2, 2022
@patrickpatrickpatrick
Copy link

This approach makes sense to me, it's what we did at a previous company where we had a CMS that could add components each with their own JS. We moved from a similar pattern of including all the scripts and initialising everything to including the only relevant javascript in script tags in the partial of each component.

Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting proposal, Ian ⭐ looking forward to seeing it!

rfc-149-switch-to-per-page-asset-loading.md Outdated Show resolved Hide resolved
rfc-149-switch-to-per-page-asset-loading.md Outdated Show resolved Hide resolved

Adding a component's CSS and JavaScript to an application's stylesheet and JavaScript is a manual process - which means that it's easy to forget to remove assets when a component has stopped being used in an application. We have auditing tools to help manage this - but they are not part of our deployment pipeline and rely on developers remembering to use them.

Supplying the assets that a component needs as part of the partial would mean that assets would only be included when the component is used - this would simplify the developer experience when adding or removing components from applications, and would also mean a better user experience since unused assets wouldn't be loaded.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big win 👏


A version identifier should be added to the component asset filename to help identify which version of the components gem is being used - for example `gem-v29-2-1-accordion.css`. Similarly, the bespoke app assets should be renamed to their application - so collections would have `collections.css` and `collections.js` - to make it easier to see which stylesheet comes from which application.

The assets would ideally be placed in a shared folder as per [RFC #91][sharing_assets], so would be able to be used across GOV.UK irrespective of the rendering application. Since this hasn't been implemented yet, there would need to be some work for this to happen - and likely this would be delayed to coincide with the replatforming work.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC 91 doesn't propose removing Rails fingerprinting. For avoidance of doubt, RFC 149 does propose removing Rails fingerprinting (for component assets), relying on semver in the filename instead - right?

For bespoke application assets, I gather you'd keep things as they are? (Currently all assets are compiled into a single application-*.css. After RFC 149, application-*.css would contain only bespoke assets). I think you're also proposing renaming application-*.css to (for example) collections-*.css but as we already have the app name in the URL, I'm not sure this is necessary 🤔

So just to confirm, post RFC-91 and RFC-149, asset URLs would look like this?

  • assets.publishing.service.gov.uk/shared/gem-v29-2-1-accordion.css
  • assets.publishing.service.gov.uk/government-frontend/application-00d6906ee26428034a3cc61e0498475b38903f895aeaf5f53222efba375a20c7.css(containing bespoke assets only)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll rewrite this to be clearer - but the tl;dr is that post RFC-91 and RFC-149 the asset URLs should look like:

  • www.gov.uk/assets/gem-v29-2-1-accordion-e03sv...98hg5.css
  • www.gov.uk/assets/government-frontend-00d69...a20c7.css

(The assets are currently served off of the www.gov.uk domain since HTTP/2 was enabled.)

The renaming of the application-*.css is because I think (though am happy to be wrong) that Rails can only use a single asset folder, which is set using config.assets.prefix1:

    # Path within public/ where assets are compiled to
    config.assets.prefix = "/assets/frontend"

If this is correct, then a Rails app wouldn't be able to use assets from /shared and /government-frontend at the same time.

Since all the applications use application-*.css and application-*.js for their bespoke assets, there'd be a lot of similarly named files where the only difference is the fingerprint - which isn't that useful when trying to tell which file is used by which application. Setting the file name to include the application name seems a good solution to that.

I'd prefer to keep the fingerprint as part of the individual assets to make doubly sure that each file with the same name has the same contents - for example, the same Sass file compiled with Dart-Sass would give a different fingerprint to one compiled with Node-Sass because they have different default number of decimal places2.

Footnotes

  1. Example taken from alphagov/frontend/config/application.rb.

  2. There's an interesting issue on how this affects GOV.UK Frontend.

Copy link
Contributor

@ChrisBAshton ChrisBAshton Aug 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I think I mostly follow. Can we look at an example just to iron out the last of the confusion?

If we take Frontend as an example and look at its use of the tabs component, it currently imports the SCSS and JS manually, combining it with all of the application assets and outputting as single files /assets/frontend/application-FINGERPRINT.css and /assets/frontend/application-FINGERPRINT.js respectively.

For stage 1, we want to implement the individual serving of assets. This would involve:

  1. Removing the import/require calls to the component
  2. Pulling in the component CSS/JS in some other way - this is where I'm stuck. It may be an implementation detail but it could end up changing parts of the proposal, so worth digging a little deeper I think. I'm not clear from the RFC how this would be done.

I think what you're proposing is roughly this:

Write a compile-time 'hook' into the render method, which copies the component CSS/JS files into the Rails project, as separate files, alongside the application asset source files.
This will then output as /assets/frontend/gem-v29-2-1-tabs-FINGERPRINT.css etc. We rely on Slimmer to yank the <link rel element and move it into the head.
The component template itself makes no reference to assets.

Then for stage 2, it's just a case of changing the config.assets.prefix value to something like /assets/shared and renaming application.scss and application.js to frontend.scss|js. Now the component would be uploaded to /assets/shared/gem-v29-2-1-tabs-FINGERPRINT.css instead.
And if another app (e.g. Whitehall) uses the same version of the same component, it will also output to the same place, essentially overwriting the contents, but we don't care since the contents (and therefore the fingerprint) would be the same. So at that point, Frontend is effectively using the tabs assets compiled by Whitehall, but there is no difference in the bytes being sent to the user.

Does that make sense? Have I understood that right?

The other alternative I can think of is to have the component templates refer to their own assets, like in the pseudocode below, but I can't think of a way of accomplishing that whilst retaining the fingerprint, and it would also need a separate build pipeline (likely a GitHub Action).

<% if use_individual_assets %>
  <link rel='stylesheet' href='<%="/assets/shared/gem-v${VERSION}-tabs.css" %>' />
<% end %>

(Might make the 'optional' logic easier though - just a case of passing use_individual_assets: false at the application level).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I think should happen. There's some discussion that should happen during the implementation about exactly how and where things should happen - but that's getting a bit too much into the weeds.

From digging into how the component gem interacts with the rendering applications I can't find anything that screams that this isn't doable. GitHub have started to break their CSS bundle up into more managable chunks - there was a presentation at this years RailsConf on it (slides) - which also makes me think that this is a achievable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rewritten the section in adbc60a - does it make more sense?

rfc-149-switch-to-per-page-asset-loading.md Outdated Show resolved Hide resolved
rfc-149-switch-to-per-page-asset-loading.md Outdated Show resolved Hide resolved
rfc-149-switch-to-per-page-asset-loading.md Show resolved Hide resolved
rfc-149-switch-to-per-page-asset-loading.md Show resolved Hide resolved
rfc-149-switch-to-per-page-asset-loading.md Outdated Show resolved Hide resolved

### Make assets load from a shared folder

Parallel to this, we need to see how RFC 91 could be implemented. This is complicated by the replatforming project nearing completion.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it? I'm not sure that replatforming will change the asset compilation/upload pipeline 🤔 I think any changes we make in this area would be carried over automatically - though @sengi will be able to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because RFC 91 hasn't been implemented there is no shared folder that shared assets could be uploaded to.

We could implement a shared folder for the current set up - but from discussions it seems likely that this would be finished just in time for replatforming to be switched on.

Possibly complicated is the wrong word - I'm try to note that the shared folder is an external factor with its own RFC, and finding out how this should be carried out is a piece of work in itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new system, Rails assets will be uploaded to a common S3 bucket on rollout (i.e. when a version of an app is deployed to the cluster). It should be feasible to share assets. Right now we're still serving files via the cluster instead of going straight to S3 from the edge (Fastly), but we'd like to do that if not before launch then soon after.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a more concrete time frame soon but we're talking this year.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's super news @sengi! Apologies, I was unaware that a shared folder was being implemented so I incorrectly assumed that it wasn't part of the new system.

I'll update this RFC to add that a shared folder is coming soon, and what this will mean for this proposal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated it to make it clearer that the shared folder is coming soon - 09a5bd2. Does this sound correct?

@maxgds
Copy link
Contributor

maxgds commented Aug 4, 2022

👍 Seems to make a lot of sense. I anticipate unseen complications as we try to shift to it but that shouldn't put us off.

@davidtrussler
Copy link

This makes a lot of sense to me. 👍

injms added 3 commits August 4, 2022 17:16
Replatforming will add a shared folder for rendering applications - huzzah - so
the RFC has been updated to reflect this.
Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ian 👍 LGTM ⭐

@DilwoarH
Copy link

DilwoarH commented Aug 9, 2022

Sounds like a good idea.

I had one question, is this just for frontend apps or publishing apps too?

@injms
Copy link
Contributor Author

injms commented Aug 9, 2022

I had one question, is this just for frontend apps or publishing apps too?

@DilwoarH I think it could be useful for any app that renders pages - whether public facing or not.

The key thing is that it should be optional - so publishing apps would still be able to use the components gem with or without this feature if that's what is wanted.

@kevindew
Copy link
Member

Great work on this proposal - lots of really useful information about HTTP and compression. It sounds like a good step reduced asset size, thanks for all the thinking and putting it together.

What’s a bit unclear to me is what the consequences of this RFC are, i.e whether this is a “we’ll spike into investigating a proof of concept as to whether this is going to work” or a “we know enough and can implement it”? Also whether this is to be done as part of a teams workstream or outside it?

The broad approach sounds good however it doesn’t seem clear to me that we’ll know whether it has some difficult compromises until there’s a proof of concept put together.

Some other thoughts/questions that come to mind are:

  • Will this change apps creating their own components? Will they have to use some govuk_publishing_components shared code to do it, or do it independently?
  • Will we continue to have separate print stylesheets for each component? will these also be individual files?
  • Following an acceptance of this RFC should teams be changing any approaches?

@injms
Copy link
Contributor Author

injms commented Aug 16, 2022

Thanks for the comments and questions @kevindew - I hope I've answered them in enough detail.

What’s a bit unclear to me is what the consequences of this RFC are, i.e whether this is a “we’ll spike into investigating a proof of concept as to whether this is going to work” or a “we know enough and can implement it”? Also whether this is to be done as part of a teams workstream or outside it?

I think that this should be rolled out properly on an app-by-app basis - then we can see the impact on real users with Speedcurve. I'm confident that the changes aren't detrimental, so doing it on an app-by-app basis will mean that if this isn't the case we can roll back the change quickly.

When the public layout component was rolled out it was done as both part of a team's work and people's individual community objectives - so I'd suggest that this should be the default approach - this was the approach for the roll out of public layout component, so we know it works. If approved, we can then go to teams to see how much space they have for this - but until there's something more concrete it's difficult to get that time allotted.

  • Will this change apps creating their own components? Will they have to use some govuk_publishing_components shared code to do it, or do it independently?

This proposal won't change how components in applications are created - only components that are shared in the gem. Some applications already make use of helpers from the gem, so if the application components were to use shared code then it wouldn't be a change to what already exists. We can then move onto looking at whether this per-template-loading change is worth doing for application components - but to keep this manageable I think that this should be out of scope of this RFC.

  • Will we continue to have separate print stylesheets for each component? will these also be individual files?

I've only looked at screen stylesheets for this RFC - unfortunately most components don't have a print stylesheet, which is a problem in itself. There needs to be a separate investigation into print stylesheets to improve this on GOV.UK.

My main question that will resolve your question is finding out when difference browsers download and parse the CSS - whether print stylesheets are eagerly downloaded after the page has loaded; when the print dialog box has been opened; or something else entirely. For this reason, I think that print stylesheets should be outside the scope of this RFC as there are too many things that aren't known.

  • Following an acceptance of this RFC should teams be changing any approaches?

Teams should be following the component principles. This RFC relies on those principles, but doesn't change them - so teams that are following the guidance (and they should be when creating components) won't have to change their approach.

In terms of how to use components in an application, this should make it easier - the CSS and JavaScript won't have to be added and removed manually, so should make the developer experience better. But as the change is backwards compatible it won't break things if both approaches are used at the same time.

Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So sorry these questions have come through late. They were supposed to come through with my comment but seem to have been stuck in a pending state.


## Proposal

Assets should be included individually on a per-component basis and included in the component partial.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to have something special for apps that use static here and components that are shared? I assume that by default we’ll end up with static including components and then a frontend app doing the same one. It looks like currently this is avoided by somewhat delicately not including static components in the list ? (which presumably means we risk the occasional breakage if rendering app and static are on different versions of the gem)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't an overlap between components used in static and the rendering apps - static only does the header and the footer, so only renders (off the top of my head) the skip-link, navigation-header, footer, banners, and feedback components. These can use individual assets and won't clash with the rendering applications as the rendering applications don't use these components.

We manually stick add components that are shared across a lot of the applications to stop the same CSS and JavaScript from being downloaded unnecessarily - this won't be needed in the future, so would stop any mismatched versions causing any breakages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So components I'm referencing are things like input, hint, label. Those that are used as part of the components static uses and are then also used in applications.

It's unclear to me how a rendering app would know that any static components used those components (well outside of hacking away at Slimmer)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure exactly how this would be done. We could:

  • have a 'components used in Static' list in the gem that we check against - which would need maintaining, similar to the current set up
  • do any deduping of stylesheets in Slimmer after the two templates have been knitted together - no list needed, but uses Slimmer which isn't ideal

I'd prefer the first option as it has no reliance on Slimmer - but any other suggestions welcome.

The other thing worth noting is that this stops becoming a problem when Static/Slimmer is not longer being used - though that's probably a while off!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I think the first option is preferable. Using slimmer makes me uncomfortable as it's very difficult to test and will likely be fragile.

The other thing worth noting is that this stops becoming a problem when Static/Slimmer is not longer being used - though that's probably a while off!

I dream of the day!


Assets should be included individually on a per-component basis and included in the component partial.

This would mean that a page loads `component.css` and `component.js` only when a component is present on the page - rather than the current behaviour, which sees component assets loaded if used anywhere within an application. If multiple uses of the same component occur on the page, there should only be one instance of the assets included on the page.
Copy link
Member

@kevindew kevindew Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some wonderings about dependencies here. Do we know:

a) what CSS/JS is expected to be global, such as polyfills, govuk-frontend core, module system? i.e. things a component can use without requiring it? Would these become separate files to application.jss/css in apps or would there be extra files and/or ordering considerations ?
b) what CSS/JS a component would expect to pull in for its own use? e.g. access common mixins in CSS case and would we always require them? (I know govuk-frontend has a two file system for dealing with the pains caused by a lack of @use)
c) will we still support an all_components file? this could be very slow if every CSS file has to load in the same dependencies.

We currently have the SCSS files of govuk_frontend_support and component_support that both output CSS and load in mixins. I’m guessing we’ll need something that does these tasks separately so apps can link to global dependencies while components have access to mixins?

I worry that if all CSS components need a common pile of imports then this will create a big performance issue (with sprockets/libsass at least) - however if the dependencies can be light this shouldn’t be a problem, but that could be annoying for mixin availability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependencies is a tricky one; JavaScript is a bit of an unknown as there are other factors outside of the RFC at play.

a) For JavaScript, I'm not completely sure. What I hope is that with the potential to drop JavaScript support for Internet Explorer 11 we can investigate the use of ES6 modules. This means that including polyfills and dependencies becomes a lot easier: import { dependency } from 'dependency'. Because the import is path based, different component's JavaScript can use the same dependency without it being downloaded multiple times - and they're only downloaded when needed rather than being bundled up whether they're needed or not. (We can look at doing even nicer things like only importing modules when a condition is met - so polyfills can be downloaded only when needed.)

For CSS, we'd include everything needed from GOVUK Frontend - apart from components - into one file and serve that; this should include all the base styles (govuk-body, govuk-heading-* etc) that are used on every single page. (This is more or less what govuk_frontend_support and component_support is.) Having this in its own file will mean that it will be more likely to be cached by the browser.

b) There's an agreement that we don't use the Sass @extend .classname feature as it creates too much junk CSS - it's smaller and simpler to just use two classes in the HTML. This means that we don't need access to the full GOVUK Frontend Sass in each component's stylesheet.

The common mixins would be included if needed by @importing the required bits from GOVUK Frontend - either by using gouvk_frontend_support or a subsection of GOVUK Frontend itself. Some components would need this, some wouldn't. This won't add anything extra to the component's stylesheet.

The lack of @use is a problem - this is exacerbated by the lack of @use support in the version of Sass that we use. Moving to Dart Sass would help build the case for GOVUK Frontend dropping libsass and RubySass support - so there is a future solution to this, but not one yet.

c) Yes, an all_components Sass file should still be supported if needed - it'd be good to know where this is used though and how much of a need there is for it.

govuk_frontend_support is only mixins and variables - it doesn't output any CSS, so can be @imported into every component without adding any extra size. I'm measuring how much impact this has when developing locally at the moment with the gem and the frontend rendering app - I do worry about the developer experience, but until we can acertain the level of impact I don't think that this should block this proposal.

component_support does output CSS - so it's only used in Static on the public facing aspect of GOVUK. This should be part of the previously mentioned stylesheet that has all the basic GOVUK Frontend stuff in it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that sounds reasonable sounds like there's a fair amount of unknown.

With JS, would we launch the individual files when we've got a similar set-up for JS and CSS. I.e individual files for each. Or would this propose doing just CSS and then leaving JS to later (so application.js entries for JS but individual files for CSS). I'm kinda hoping it'd be the former.

One of the key places of all_components is in the component-guide itself, otherwise I think it's in backend apps that haven't had much love recently (Content Publisher, Content Data etc)

If this work brings us closer towards Dart and/or modernising the JS that'd be a good outcome

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd do this for CSS first, and then tackle the JavaScript afterwards - so, as you say, there'd be application.js and individual CSS files.

Whilst it's not perfect, even just serving the CSS individually would be an improvement - and I worry that doing both at the same time would be too big a chunk of work to get done in a sensible time frame.

Waiting for clearer direction for IE11 support, the upcoming translation work from GOVUK Frontend, and investigation into how the latest version of Rails uses ES6 modules would mean a longer wait until the JavaScript is loaded individually, but potentially less work to get it done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh sure, that does seem somewhat messy as we'd have a separate install step for the JS and stylesheet.

It seems ok to go with that as a first step and agree with your point about sensible time frame, but I really wouldn't wait for ES6 modules as we've got so many things to resolve to get to that. I imagine we could just create an equivalent JS file of component support and then just add each component as extra JS, similar to CSS.

As an aside, I do wonder how we can start work on what JS without IE support looks like and the problems to overcome.

rfc-149-switch-to-per-page-asset-loading.md Outdated Show resolved Hide resolved

This balance between making the site load faster for a majority of users is not just a numbers game - the users who are most likely to be using older browsers are those that are likely to least afford the upgrade cost for a newer device, and they're likely to be the users who most need the information on GOV.UK.

Whilst this proposed change will make things slower for users whose browser doesn't support HTTP/2 or HTTP/3, it won't prevent pages from loading. This proposed change should be accepted because it will dramatically improve page performance for almost all visitors without blocking access for a small number of visitors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any estimates of how dramatic the improvement will be? I'd have thought this would only be of significance on slow connections/devices but would love to hear otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've included how much less CSS will be downloaded on the top 100 pages - all of the pages showed a reduction in the amount of CSS serve.

But since the other part of this is to improve the cacheability of the files I don't think we'll be able to tell that until we've look at the real user metrics. A bit of a cop out I'm afraid!

If you're on a fast-ish wired connection you probably won't notice any difference. If you're on a mobile connection it should really help, especially if it's flaky with a risk of both dropped packets and connections.


Because each application is using the components gem as the source for the assets for each component, the compiled assets will be the same - whether the resulting CSS or JavaScript file is produced by whitehall, frontend, or collections. When writing to the shared folder the component assets may be overwritten, but - thanks to the fingerprint - they'll be overwritten with files containing the same content.

## Considerations
Copy link
Member

@kevindew kevindew Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There probably should be something on impact to developer tooling as a consideration.

I am a bit concerned about the impact we'll have on asset compilation times with the libsass / sprockets combination. We had to do a lot of work in the past to get these issues under wrap as they lead to failed test runs, apps that don't seem to run on first request etc once you end up with asset compilations drifting from seconds to minutes.

There's a clearly a balance between application performance and ability to work with the application. So I'm wondering do we know yet enough about what the impact on development experience will be and, if not, if we're able to establish what would be an unacceptable regression?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should be an optional choice and backwards compatible - so if that means running a accordion-individual.scss file that @imports from govuk_frontend_support and from the _accordion.scss then that's okay. I'm aware that there are applications that don't want this approach, and the gem needs to work (and work without taking a long time) for both publishing and rendering applications.

A quick check of frontend running using app-live in GOVUK Docker, cold start:

  • using gem: 17 seconds
  • using local version of the gem: 44 seconds
  • using local version of the gem with @import govuk_frontend_support for each component that requires it: 59 seconds

Whilst not great, I think it's an acceptable increase whilst there are other avenues worth investigating to make this faster - off the top of my head, more efficient ways of only importing the specific things that are needed from GOVUK Frontend, using the faster Dart Sass, use of @use within the components gem and rendering applications.

This is only a single run, so I'll do some more investigation into this tomorrow to be able to report an average number of runs.

Whilst not a complete solution, running it using ./startup.sh --live seems to be about four times faster, which makes me suspicious that I'm not doing a completely cold start.

I'd suggest that an unacceptable regression would be if a cold start is above a minute under the current libsass and Docker set up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Yup having files without needing the import sounds great to avoid performance penalties everywhere. If it's a 25% increase as per your experiment that isn't too bad either - it's an order of magnitude better than I feared.

A minute sounds good too, so long as we have a certain point where we're prepared to go "eep maybe we should focus on getting dart in first" works for me

Yeah to do a full cold start you need to clear the cache, rails assets:clobber tmp:clear I think to be complete

Instead added it to the list of things that were considered and rejected.
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together. This has broad support throughout the comments so we can consider this accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants