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

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

Closed
wants to merge 1 commit into from

Conversation

36degrees
Copy link
Contributor

@36degrees 36degrees commented Mar 17, 2020

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.


📖 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

@36degrees 36degrees force-pushed the remove-imports-from-components branch from 123ba76 to 1e3d90d Compare March 17, 2020 15:56
@36degrees 36degrees changed the title Propose removing imports from components Remove the imports of settings, tools and helpers layers from components Mar 17, 2020
@penx
Copy link
Contributor

penx commented Mar 17, 2020

This won't be great for JavaScript applications that import the scss files directly:

This example is in React (which could be used server side only) but applies to other JavaScript server side and client side frameworks:

App.js

import React from "react";

import "./settings.scss";
import "./component.scss";

export default function Component() {
  return <div className="my-component">Test</div>;
}

component.scss

.my-component {
  border: 1px solid $example;
}

settings.scss

$example: blue

Returns:

Error: Undefined variable: "$example".
        on line 2:21 of /stdin
>>   border: 1px solid $example;

https://codesandbox.io/s/govuk-design-system-architecturepull21-5x5f4

In order to make this work, an application that otherwise doesn't have any custom CSS, would need to manually make an SCSS file for every component they intend to use within their application.

How about, instead, for each component in govuk-frontend, having:

  1. a file called accordian/_accordion.scss (or accordian/_index.scss)
    This file has the required includes and is what users of component only css would use. This file also imports file 2:

  2. another file called accordian/_component.scss (or if using _index.scss for 1, call this accordian/_accordion.scss)
    This file contains the component only scss with no imports.

Then have govuk/all only import the component only scss, but leave the existing component only functionality working as it does currently

@36degrees
Copy link
Contributor Author

Thanks @penx – that's really helpful feedback.

We explored something similar in alphagov/govuk-frontend#1752, but we ended up deciding against it. However, we'll take another look in the context of the example you've provided 👍


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.

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:

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 this @36degrees really pleased to see this coming through 👍

I think the libsass numbers are potentially a little misleading as that's just bare libsass whereas the numbers have been much more dramatic for libsass through Ruby and libsass through webpacker.

```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/helpers/all";

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

@benthorner
Copy link

Hey everyone 👋. I'm wondering how this is going? The due date was quite a while ago, and I expect that may have something to do with coronavirus! It would still be really helpful if we could make some progress here. We're still suffering from slow demos and slow tests.

There only seems to be one blocking issue on this card, related to using the design system in a JS/webpack context, where each import is processed in isolation. Arguably these kinds of use cases are trying to treat SASS like CSS, when they're fundamentally different.

However, we'll take another look in the context of the example you've provided

@36degrees what other approaches were we considering? In case it helps, I have a couple of suggestions, although my knowledge of this domain is quite limited:

  • If there's a need to consume design system as CSS, could/should we meet that need directly?
  • Someone else appears to have solved this problem using prependData for sass-loader

In general, it would be good to understand what more we need to do to move forward with this proposal (assuming it's viable).

@hannalaakso
Copy link
Member

Hi @benthorner

We're currently working on a revised proposal on the basis of the feedback we got on this proposal. We'll be posting the new proposal in this repo by the end of this week.

Thanks for your patience!

@36degrees
Copy link
Contributor Author

@penx @theseanything @kevindew @benthorner Thank you all for your feedback on this proposal.

Based on your comments, we've changed our approach and raised a new proposal, which replaces this one. It'd be great to get your feedback on it.

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.

6 participants