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.

Sign up for GitHub

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

Remove the imports of settings, tools and helpers layers from components #21

Closed
wants to merge 1 commit into from
Closed

Remove the imports of settings, tools and helpers layers from components #21

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions proposals/007-remove-imports-from-components.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# Remove the imports of settings, tools and helpers layers from components

**Closing date:** 27 March 2020

**Status:** Proposed

## Summary

We plan to remove the imports of the settings, helpers and tools layers from all components in GOV.UK Frontend version 4.0.

We’re doing this to reduce the time it takes to compile GOV.UK Frontend’s Sass to CSS, especially if you’re using the Ruby Sass compiler.

## Problem

If you're using the Ruby Sass compiler to compile Sass to CSS, it can take a long time because GOV.UK Frontend has a lot of imports.

Although Ruby Sass is deprecated, teams (including GOV.UK) are stuck using it for the foreseeable future.
Copy link

Choose a reason for hiding this comment

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

It would be good to have background on why teams are stuck with Ruby Sass? Looking at the benchmarks below, it doesn't seem like huge issue with the other compilers - i.e. 0.02s, 0.25s less seems hardly noticeable. Should we be focusing on moving away from Ruby Sass instead?

Copy link
Member

Choose a reason for hiding this comment

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

There is a big problem with us (GOV.UK) switching to libsass with govuk-frontend having the large number of imports. This reduces the time of initial compile by about 50% but then has a massively slow time for repeated requests (in Finder Frontend on govuk-docker a warm libsass cache took 17s responses compared to 0.22s with RubySass). I'm pretty sure this is all a problem with Sprockets' usage of libsass rather than any issues in libsass and it's rather unfortunate as it's a real bummer still using RubySass.

I don't think it's quite a forseeable future issue though I think within a few months if we can dedicate some resources it's resolvable (depending on the current crisis) there has been talking about putting together a frontend dev based group together to regularly catch up on this.

Aside from the big warm cache issue (which would be mostly resolved by this proposal) the other issues we've got are:


If we make these changes, we'll also improve the compile time in other Sass compilers. Approximately, we expect to see these improvements:

| Compiler | Compile time now | Compile time after proposed changes | Improvement |
|-----------|------------------|-------------------------------------|-------------|
| Ruby Sass | 30s | 7.8s | 74% |
| LibSass | 0.2s | 0.18s | 10% |
| Dart Sass | 0.5s | 0.25s | 50% |

For more context, see [#1671 Slow compilation of SCSS files in Rails apps](https://github.com/alphagov/govuk-frontend/issues/1671).

## Proposal

We propose removing the following imports of the settings, helpers and tools layers from each component:

```scss
@import "../../settings/all";
@import "../../tools/all";
@import "../../helpers/all";
```

This will have no effect if you currently import `govuk/all`, as all of the layers are already imported in order.

If you selectively import one or more components, you would need to make sure you import the settings, tools and helpers layers first:

```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.

Would it be possible to have a single import that can bring in all the base dependencies? I know this means reaching into the dreaded world of generic naming for things like "govuk/base", "govuk/common" or "govuk/dependencies". As I'd expect only a very knowledgeable power user would want to be distinguishing between settings, tools and helpers.


@import "govuk/components/accordion/accordion";
@import "govuk/components/button/button";
Copy link
Member

Choose a reason for hiding this comment

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

Not really related to the proposal but I wonder if you wanted to use this bc break as an opportunity to add files like govuk/components/button/_index.scss so they could be required without the "button/button"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely worth exploring, but if we wanted to take advantage of that internally we'd need to drop support for LibSass < 3.6.0 and Ruby Sass < 3.6.0 (according to https://sass-lang.com/documentation/at-rules/import#index-files) – may be worth it, but we'd need to communicate that.

```

As this changes the way that some users would import GOV.UK Frontend, this would be a breaking change, which we'd introduce in GOV.UK Frontend version 4.0.