`_warning` mixin cannot be used within functions

querkmachine commented 1 year ago

Description of the issue

We use a _warning mixin in Sass to standardise our deprecation warnings and provide a mechanism for service teams to disable intrusive and repetative warnings.

Our documentation for 'managing change' includes a section about using the mixin within a function.

However, it isn't actually possible to call a Sass mixin from within a function. Doing so returns an error in all Sass compilers that we currently support in Frontend:

Dart Sass — This at-rule is not allowed here.
libsass — Functions can only contain variable declarations and control directives.
Ruby Sass — Functions can only contain variable declarations and control directives.

Steps to reproduce the issue

Try to include the _warning mixin (or any mixin) from inside of a function.

Actual vs expected behaviour

Presumably the documentation is incorrect and needs to be amended.

We may need to devise a method of producing (and suppressing) warnings that are spawned by functions.

Environment

GOV.UK Frontend version: 4.5.0
Dart Sass version tested: 1.32.12
libsass version tested: 3.5.5
Ruby Sass version tested: 3.7.4

owenatgov commented 1 year ago

Good catch. Really this should be a function, I'm not sure why it's not 🤔

querkmachine commented 1 year ago

Probably because being a mixin is the only way that does it 'right'.

It seems like Sass only likes function calls to be used in certain ways. Changing the _warning mixin to a function and trying to call it stadalone like this produces a syntax error on compile:

@function get-colour {
  @if true {
    _warning("doo-wop", "woo-bop");
  }
  @return #786999;
}

body {
  color: get-colour();
}

Changing it to use the @return statement resolves the syntax error, but also negates its use as a warning, because the function will no longer return the colour — 'breaking' the output until the source of the warning has been fixed.

@function get-colour {
  @if true {
    @return _warning("doo-wop", "woo-bop");
  }
  @return #786999;
}

body {
  color: get-colour();
}

Trying to use it within a mixin will also only work if it's attached to a property (real or not).

@mixin nice-colours {
  warning: _warning("doo-wop", "woo-bop");
  color: white;
  background-color: #786999;
}

body {
  @include nice-colours;
}

Using functions to manipulate global variables is also frowned upon in the Sass docs (see first "Head's up!"), which says to use mixins instead, though it's technically possible.

Bootstrap also uses a mixin for deprecation warnings and (appears) to claim that it can be used with functions, however it doesn't seem to be used in the same way. It looks like they use it only for deprecating entire functions, and do so by including the mixin in the same file—outside of the function definition—rather than as an include within the function definition.

owenatgov commented 1 year ago

Thanks for the writeup here.

Boostrap's approach is really interesting (ref). As you say, their practice is to put the definition outside the thing being deprecated, I presume because it hits at the point of import rather than at the point of use. I'm personally not sure this is a great approach for us as we don't split our stuff out so explicitly at the moment and teams are generally more likely to just import the whole project rather than segment their stuff, especially for prototypes.

A solution to this could be to be to just do that, split everything out, but a quicker win for now might simply be to update the docs on deprecation to specify that the warning mixin won't work with functions and you'll need to interact with the API manually 😞 It's not fantastic but it's a start in the absence of a better solution.

owenatgov commented 1 year ago

We had a chat about this on slack during the review phase of https://github.com/alphagov/govuk-frontend/pull/3576 and a few ideas emerged. Generally, we think that making the different pieces of _warning more portable will make this easier to use. Specifically:

moving the text generated for the @warn in _warning into a function and passing that into the @warn
moving the logic that checks if a kay has been added to $govuk-suppressed-warnings into it's own function

This could look something like this:

@mixin _warning($key, $message) {
  @if _should-warn($key) {
    @warn _warning-text($key, $message);
    $govuk-suppressed-warnings: append($govuk-suppressed-warnings, $key) !global;
  }
}

@function _should-warn($key) {
    @return index($govuk-suppressed-warnings, $key) == null;
}

@function _warning-text($key, $message) {
  @return $message + " To silence this warning, update $govuk-suppressed-warnings " +
  "with key: \"#{$key}\"";
}

... and would mean instances where the mixin couldn't be used could instead be replicated like so:

@if _should-warn("my-warning-key") {
    @warn _warning-text("my-warning-key", "This is a warning message");
}

There's additionally a question around if the line in _warning that stops duplicate warnings per sass compile is useful or not.

querkmachine commented 1 year ago

I've opened a PR that implements the above suggestions and updates the offending documentation. Feel free to pick it up in my absence.

alphagov / govuk-frontend