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

Simplifying imports within GOV.UK Frontend to improve compilation performance #22

Merged

Conversation

36degrees
Copy link
Contributor

This is an iteration of a previous proposal, based on the feedback we received.

We're planning to make changes to the way that the base layers (settings, tools and helpers) are imported within different parts of GOV.UK Frontend’s Sass, to reduce the time it takes to compile to CSS.

Most of you will not be affected by these changes, and if you import "node_modules/govuk-frontend/govuk/all" you will not need to do anything to benefit from these performance improvements.

It will have the most impact on you if you import only specific parts of GOV.UK Frontend.


📖 Read the proposal

💬 Add comments to this pull request, or on the proposal itself

✉️ If you prefer, send feedback to govuk-design-system-support@digital.cabinet-office.gov.uk

👍 You can also +1 this pull request if you agree with the proposal and have no other feedback to give

@penx
Copy link
Contributor

penx commented Apr 24, 2020

Thanks @36degrees, looks like this will resolve my previous feedback!

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.

This sounds good to me 👍

```scss
@import "govuk/settings/all";
@import "govuk/tools/all";
@import "govuk/helpers/all";
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to repeat a question I asked before I'm just wondering if we can combine these dependencies into a single import.

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 this is definitely something we'll consider, but we didn't want to add too much 'noise' to the proposal. I'm seeing it as an implementation detail at this point.

Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

I'm not sure I'm being of any help with my comment, but I'll leave it in case someone feels the same.

This proposal is good as a short term solution (and I highly appreciate that the team dedicated time to look into it), in a sense that it seems to be helping with improving the Rails assets compilation times, as it is addressing/mitigating the effects of @import rule by using it more strategically, but on the long run I would prefer us to move towards @use and the new way of doing modules in SCSS – and this would be, in my humble view, a more justified way of having a breaking change (v4) for the way we use SCSS modules.

As @36degrees showed in #1752, GOV.UK would benefit from switching from the blackhole of Ruby Sass to LibSass (30s -> 0.2s) much more than this change in govuk-frontend would bring (0.2s -> 0.18).


## Problem

It can take a long time to compile GOV.UK Frontend’s Sass to CSS. This is because many of the individual files within GOV.UK Frontend import their own dependencies, even if they would usually have already been imported. We originally did this so that you could import specific bits of GOV.UK Frontend without having to separately import their dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

We originally did this so that you could import specific bits of GOV.UK Frontend without having to separately import their dependencies.

I'm on the opinion that we should continue to value the principle of importing specific bits without having to separately import dependencies. The fact that the legacy sass implementation (where 'modularisation' was only possible with @import and @export) didn't allow any sort of 'import-once'/'tree-shaking' was overcome later on with @use which allows us to preserve this principle.

@36degrees
Copy link
Contributor Author

36degrees commented Apr 28, 2020

This proposal is good as a short term solution (and I highly appreciate that the team dedicated time to look into it), in a sense that it seems to be helping with improving the Rails assets compilation times, as it is addressing/mitigating the effects of @import rule by using it more strategically, but on the long run I would prefer us to move towards @use and the new way of doing modules in SCSS – and this would be, in my humble view, a more justified way of having a breaking change (v4) for the way we use SCSS modules.

This is definitely on our roadmap, but at the minute only Dart Sass supports @use. I don't think we can safely drop support for LibSass as I imagine this will leave a lot of our users on e.g. 3.x, without the motivation and the capacity to migrate to a new Sass compiler.

For that reason, we can't move to the module system until LibSass has had support for it for at least 3-6 months, and the related issue looks like it's currently tracking against LibSass 5.0 – for context, the latest release is 3.6.3.

(The 'future plans' section of the module system blog post sets a hard limit of October 2021 for deprecating @import, which we could also do, but we'd need to have a better understanding of the impact it'll have on our users if LibSass still hasn't implemented @use by then!)

Realistically, I think this pushes a migration to @use back till 2021. I think we should make this more 'visible' to users, so I'll look at creating an issue to allow us to track this.

In the meantime, the proposed changes for 4.0, although breaking, will only affect users who currently import from the core or overrides layer without importing the dependencies. I think the risk of these changes is pretty low?

However, most of the performance gains do come from the changes to the components, so we could consider leaving the other layers alone if we don't think the benefit is worth it.

As @36degrees showed in #1752, GOV.UK would benefit from switching from the blackhole of Ruby Sass to LibSass (30s -> 0.2s) much more than this change in govuk-frontend would bring (0.2s -> 0.18).

My understanding (based on @kevindew's comment here – #21 (comment)) is that without reducing the number of imports LibSass when used with Sprockets also shows poor warm-cache compilation times. Therefore, reducing the number of imports is still beneficial to GOV.UK. My benchmarks were using libsass in isolation, and only considered behaviour with a cold cache.

Co-Authored-By: Alex <alex-ju@users.noreply.github.com>
@36degrees
Copy link
Contributor Author

36degrees commented Apr 28, 2020

Realistically, I think this pushes a migration to @use back till 2021. I think we should make this more 'visible' to users, so I'll look at creating an issue to allow us to track this.

I've raised an issue on GOV.UK Frontend to track migrating to @use: alphagov/govuk-frontend#1791

@alex-ju
Copy link
Contributor

alex-ju commented Apr 28, 2020

thank you for the thorough reply @36degrees. it's good to know modules are on the roadmap. I know they're not there yet and even if @forward is meant to provide backwards compatibility it has some limitations at the moment. I'm happy to see this in, just wanted to make sure the teams' efforts are balanced.

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.

5 participants