Migrate to Dart Sass from LibSass

[jon-kirwan]

What

Migrate to Dart Sass from LibSass.

Why

• LibSass is now deprecated
• Making changes ahead of Design System team dropping support for LibSass Dropping support for Ruby Sass and LibSass govuk-frontend#2632.

Visual Changes

None.

How can I test these changes?

From GOV.UK Publishing Components

• run yarn jasmine:prepare (or run bundle exec rake app:assets:clobber app:assets:precompile)
• what should happen? The Sass files should be compiled. Keep an eye on the command line for warnings indicating the compilation process using Dart Sass

Deprecation Warning: Using / for division outside of calc() is deprecated and will be removed in Dart Sass 2.0.0.
Recommendation: math.div(300%, 4) or calc(300% / 4)
More info and automated migrator: https://sass-lang.com/d/slash-div
   │
26 │   three-quarters: (300% / 4),
   │                    ^^^^^^^^
   ...

• the /builds folder: app/assets/builds should now be populated with CSS files
• subsequently, all assets ought to be generated successfully within the /public directory
• no test failures to indicate the correct number of stylesheets are generated
• run the Component Guide ​​./startup.sh --live. Inspect several pages, e.g. accordion, public layout, related navigation, to ensure the absence of errors (Percy would catch any visual diffs but might be useful to have a quick look anyway). You can compare with the live Component Guide.

From Email Alert Frontend

See alphagov/email-alert-frontend#1655.

From Email Alert Frontend (verify correct manifest is used when sassc-rails is bundled)

See alphagov/email-alert-frontend#1657.

Watch for changes

Sass now stays open and continues compiling stylesheets whenever they or their dependencies change https://sass-lang.com/documentation/cli/dart-sass/#watch:

• run ​​./startup.sh --live and browse to the component guide homepage
• open app/assets/stylesheets/govuk_publishing_components/components/_title.scss and make the following change:

.gem-c-title__text {
+ color: red;
  margin: 0;
}

• reload the page; the title text for "Component Guide" appears in red.

Anything else

Once the PR gets the green light, any temporary commits and references to local gems e.g. gem "govuk_publishing_components", path: "../govuk_publishing_components" will be removed.

See:

• Migrate to Dart Sass from LibSass collections#3447
• Migrate to Dart Sass from LibSass email-alert-frontend#1655
• Migrate to Dart Sass from LibSass static#3190
• Migrate to Dart Sass from LibSass govuk-developer-docs#4216.

[MartinJJones]

The changes look great to me overall, nice work!

Other than the small nitpicky comments on variable names, I did have one question on how we will maintain backwards compatibility with libsass in the gem.

For example, we have previously disabled stylelint rules because libsass was not able to compile the CSS as it did not understand the Media Feature Range notation syntax

I have only seen one example of this in the gem in _media-down.scss.

I imagine it would be possible to use the new syntax (or other new features in the future) and get no compilation errors if using DartSass, but this would likely result in a breaking change if you are still using LibSass.

Overall, I still feel like this is the right way forward, and maybe this is just something we can make clear in the comments and any related documentation.

[jon-kirwan]

Other than the small nitpicky comments on variable names, I did have one question on how we will maintain backwards compatibility with libsass in the gem.

For example, we have previously disabled stylelint rules because libsass was not able to compile the CSS as it did not understand the Media Feature Range notation syntax

I have only seen one example of this in the gem in _media-down.scss.

I imagine it would be possible to use the new syntax (or other new features in the future) and get no compilation errors if using DartSass, but this would likely result in a breaking change if you are still using LibSass.

I looked into this, and it seems we're fine without any more changes. I did the following:

• removed the stylelint comments from _media-down.scss. run yarn run lint:scss and no linting errors are detected (which is correct)
• I then made a similar change in Email Alert Frontend (when using the sassc-rails gem); copied the invalid media query mixin; as expected I see stylelint errors:

9:14  ✖  Expected "context" media feature range notation  media-feature-range-notation

• however, I see no stylelint errors or warnings for govuk_publishing_components
• after removing the copied mixin and attempting again, no errors were found this time
• I think this is because stylelint will only examines the Sass files within the repository it's executed on, excluding assets/stylesheets/govuk_publishing_components/components.

Thanks for pointing this out. I hadn't considered it. I'll create an issue or a backlog card specifically for removing the disabled stylelint comments.

[1pretz1]

Nice work Jon! Looks good from my end. Left one minor comment which should hopefully simplify maintaining the component stylesheets.

[1pretz1]

Instead of hardcoding each asset, could we just loop over the assets and pop it in a method? (Also notice _copy-to-clipboard.scss is missing, although doesn't contain any styling so may be intentional).

Something like:

files = Dir.glob("#{GovukPublishingComponents::Config.gem_directory}/app/assets/stylesheets/govuk_publishing_components/components/**")

scss_files = files.each_with_object({}) do |file, hsh|
  next if File.extname(file) != ".scss"

  hsh[file] = file.split("/").last(3).join("/")
end
      
scss_files["#{GovukPublishingComponents::Config.gem_directory}/app/assets/stylesheets/component_guide/application.scss"] = "component_guide/application.css"

scss_files

[jon-kirwan]

Thanks for the review and feedback @1pretz1! 👍

@MartinJJones and I have discussed this one previously and thought that listing all the stylesheets would a better way to replicate the manifest and Dart Sass initializer and by individually listing each stylesheet, it might provide an easier overview of the included stylesheets at a glance.

Also, excluding _copy-to-clipboard.scss is intentional since, as you mentioned, it’s an empty stylesheet; thereby eliminating an unnecessary network request.

I agree it's not ideal and looping through the directory contents would be nicer (potentially less prone to errors as well). However, considering the reasons mentioned above, what are your thoughts on this?

[1pretz1]

Ah good to know this has already been discussed and I see where you both are coming from. My view is that we probably want to avoid hardcoding a long list that we could just generate from a directory, as it means one less file to update when components are added / removed / renamed. The current list is also a bit unwieldy due to the amount of lines and line length IMO.

I think as the list comprises of solely stylesheets from the components dir (+ application.scss) then it shouldn't be any harder to glance at that directory vs scan this list we have defined in the config file.

I've had a more refined attempt with the purpose of being as explicit as possible to convey the intent of the code - what do you think? I'm also happy to be persuaded against a loop if you and Martin think an explicit list has more advantages than disadvantages.

gem_directory = GovukPublishingComponents::Config.gem_directory
component_stylesheets_to_include = "/app/assets/stylesheets/govuk_publishing_components/components/*.scss"
component_stylesheets_with_gem_dir = Dir.glob(gem_directory + component_stylesheets_to_include)
application_stylesheet_with_gem_dir = "#{gem_directory}/app/assets/stylesheets/component_guide/application.scss"

stylesheets_hash = { application_stylesheet_with_gem_dir => "component_guide/application.css" }

stylesheet_files_with_gem_dir.each_with_object(stylesheets_hash) do |file_path, hsh|
  hsh[file_path] = "govuk_publishing_components/components/" + File.basename(file_path)
end

[jon-kirwan]

Thanks @1pretz1. Thinking about it some more you're right that managing it this way would be more manageable and we had concerns about including an extensive list of files, wondering if there might be a better way or a better place to manage them; we do have tests in place to ensure the correct number of stylesheets are generated.

I’ve included the code above and made a couple of small changes to:

• use the gem_directory method already included in the config file
• exclude copy-to-clipboard and
• changed the filename extension for each generated file to css (from scss).

[1pretz1]

Nice @jon-kirwan - happy with the minor changes 👍 I've made a slight tweak to the code which makes it a little more readable. Concerning the empty copy-to-clipboard file, I'm assuming there is a valid reason for its existence?

stylesheets = Dir.glob("#{gem_directory}/app/assets/stylesheets/govuk_publishing_components/components/*.scss")
# _copy-to-clipboard.css is removed due to being an empty stylesheet
files_to_include = stylesheets.reject { |path| path.include?("copy-to-clipboard") }

application_stylesheet_hash = { "#{gem_directory}/app/assets/stylesheets/component_guide/application.scss" => "component_guide/application.css" }

files_to_include.each_with_object(application_stylesheet_hash) do |path, hsh|
  hsh[path] = "govuk_publishing_components/components/#{File.basename(path, '.*')}.css"
end

[jon-kirwan]

Great, that looks clearer to me! I've added separate commits (will squash into the "add all_assets config" if all looks okay).

Regards the copy-to-clipboard stylesheet it appears to have been created back in 2018 #494. My guess is that it was added to meet testing criteria, ensuring conformity with component conventions, even if, in this instance, the component doesn't contain any styles.

[1pretz1]

Awesome, all looks good to me and thanks for clarifying. Seems a bit confusing to maintain an empty stylesheet (+ the logic it creates in the method) but this isn't a concern of the PR 😄

[MartinJJones]

Nice work!, frontend changes and the recent improvements to getting the list of stylesheets good to me 👍

[1pretz1]

Looks good to me, nice work! 🍏