Silence Sass deprecation warnings coming from GOV.UK Frontend by treating Sass files loaded from extensions as dependencies

[joelanman]

Dart Sass allows you to silence deprecation warnings coming from dependencies (like GOV.UK Frontend, which is included via the extensions system) by using the quietDeps flag. A ‘dependency’ is defined as any file that’s loaded through loadPaths or importer. Because we are using the legacy Javascript API, we use includePaths which is equivalent to loadPaths.

In order to ensure that Sass files loaded by the extension system are treated as dependencies, we need to load them using this mechanism.

Add node_modules to the list of include paths when calling the Sass compiler, and modify the import paths generated by the sass-extension so that they are relative to node_modules rather than full file-system paths.

Update the two Sass files that manually import GOV.UK Frontend to import them relative to node_modules.

Enable the quietDeps flag and remove the silent logger as it is no longer needed.

We also have to update to Dart Sass 1.49.10 which allows us to silence the remaining warnings, which are caused when mixins and functions in GOV.UK Frontend are called from application stylesheets.

From the changelog for 1.49.10:

Quiet deps mode now silences compiler warnings in mixins and functions that are defined in dependencies even if they're invoked from application stylesheets.

[nataliecarey]

I've tested this locally and I'm happy that it works for Extensions and shows errors for regressions inside our codebase.

[36degrees]

I can rebase this and try and resolve the conflicts this morning.

[lfdebrux]

@36degrees I definitely like these changes... I'm a little worried though about changing the Sass import paths. This feels like it could be a breaking change for some people, especially for people using other extensions. Struggling to find an example so far though, so I could be worrying for nothing 😅

[36degrees]

I'm a little worried though about changing the Sass import paths. This feels like it could be a breaking change for some people, especially for people using other extensions. Struggling to find an example so far though, so I could be worrying for nothing 😅

As far as I can tell no import paths were previously defined, and so these changes should just be adding 'node_modules' to what was previously an 'empty set'. Therefore, I think any previously working import would have had to resolve relative to the stylesheet and not rely on import paths (because there weren't any).

So based on that and the comment here, I can't think of a scenario where the behaviour of an existing working import would be affected by this change… 🤔

Does that make sense to you, or am I missing something?

[lfdebrux]

I'm a little worried though about changing the Sass import paths. This feels like it could be a breaking change for some people, especially for people using other extensions. Struggling to find an example so far though, so I could be worrying for nothing 😅

As far as I can tell no import paths were previously defined, and so these changes should just be adding 'node_modules' to what was previously an 'empty set'. Therefore, I think any previously working import would have had to resolve relative to the stylesheet and not rely on import paths (because there weren't any).

So based on that and the comment here, I can't think of a scenario where the behaviour of an existing working import would be affected by this change… 🤔

Does that make sense to you, or am I missing something?

Ah, maybe my language is a bit imprecise here sorry; what I was specifically referring to was that this PR changes the path you need to use to @import a Sass file in the node_modules tree (so I guess it's actually about load paths/include paths, rather than import paths). My thinking was that if someone has a Sass file that they're importing in the same way that we do in docs.scss

https://github.com/alphagov/govuk-prototype-kit/pull/1272/files#diff-7483aebf0f5093509cb6d0bec0bc39a40f879df4f0ec4a6e5cfa3dc8bc8e956a

- @import "node_modules/govuk-frontend/govuk/all";
+ @import "govuk-frontend/govuk/all";

then they would have to change their import path in the same way.

Fortunately it looks like if you're using an extension, the way the _extensions.scss file is generated means that you don't need to add your own Sass import (I found a good example in a DEFRA prototype that uses both govuk-frontend and hmrc-frontend, although I'm not sure how their _extensions.scss file got committed 😅).

[36degrees]

Based on my understanding, I think the existing import would continue to work, it's just that the deprecation warnings wouldn't be silenced as it wouldn't be treated as a dependency. From memory (it was a while ago now) that's the only reason I had to change those hardcoded paths.

[lfdebrux]

Based on my understanding, I think the existing import would continue to work, it's just that the deprecation warnings wouldn't be silenced as it wouldn't be treated as a dependency.

Ah of course, that makes sense. Except...

From memory (it was a while ago now) that's the only reason I had to change those hardcoded paths.

... if I change the paths back (but keep the other changes) I don't get warnings 😕

[36degrees]

Possibly I'm mis-remembering then. Perhaps I made that change expecting the remaining warnings to be silenced, before realising that Dart Sass 1.49.10 was required to 'silence compiler warnings in mixins and functions that are defined in dependencies even if they're invoked from application stylesheets'?

Maybe Sass is clever enough to realise that even though the path is relative it's still loading a path that could be loaded using loadPaths?

[lfdebrux]

@36degrees thanks for spending time looking at this with me this morning! This comment summarises what I think we figured out from that:

I think we found that the change to add includePaths and import GOV.UK Frontend via a load path is not strictly necessary. I think we found that Dart Sass treats imports relative to the current working directory as different to imports relative to the file in which the @import rule appeared, quietDeps works the way we want either with that change or without it.

This can be seen by taking the changes in branch https://github.com/alphagov/govuk-prototype-kit/tree/ldeb-dart-sass-quiet-deps (which doesn't add includePaths, but has the same outcome as this PR), and change the import in app/assets/sass/unbranded.scss to be relative to the file:

- @import "node_modules/govuk-frontend/govuk/settings/all";
+ @import "../../../node_modules/govuk-frontend/govuk/settings/all";

then you get warnings about division where previously there were none.

So I think while it is correct that Dart Sass treats any file that's loaded through loadPaths as a dependency [1], the behaviour of the default importer [2] seems to also need to be taken into account (except with the caveat that 'stylesheets that are imported relative to the entrypoint are not considered dependencies').

---

So with all that in mind, I've raised a PR against my own branch, without the loadPaths change, and I'd like to merge that instead, on the principle that that requires less changes and having less code is better 😅

Does that all make sense to you? Would you be happy to review PR #1301?

[lfdebrux]

Closing in favour of #1301 (can reopen if we decide not to go that way)