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

Fix or suppress Dart Sass deprecation warnings #3192

Merged
merged 3 commits into from
Feb 21, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Jan 20, 2023

This PR fixes or suppresses all deprecation warnings that show up in Dart Sass

For forwards compatibility (LibSass, Node Sass, Dart Sass) not all deprecations can be resolved

Breaking change fixes

$string: red is not a string.

Dart Sass will throw when trying to quote() colour identifiers as it no longer treats them as unquoted strings. We can use interpolation "#{$colour}" so Dart Sass takes red (color) as "red" (string)

See “Heads up!” box on Strings (Unquoted) docs:
https://sass-lang.com/documentation/values/strings#unquoted

Error: $string: red is not a string.

39 │   $colour: quote($colour);
   │            ^^^^^^^^^^^^^^

    helpers/_colour.scss 39:12  govuk-colour()
    - 13:16                     root stylesheet]

Deprecations fixed

$weight: Passing a number without unit % (30) is deprecated.

The fix? Add units where needed

Deprecation Warning: $weight: Passing a number without unit % (30) is deprecated.

To preserve current behavior: $weight * 1%

More info: https://sass-lang.com/d/function-units


83 │   @return mix(#000000, $colour, $percentage);
   │           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

    src/govuk/helpers/_colour.scss 83:11               govuk-shade()
    src/govuk/components/tag/_index.scss 38:12         @content
    src/govuk/tools/_exports.scss 29:5                 govuk-exports()
    src/govuk/components/tag/_index.scss 1:1           @import
    src/govuk/components/phase-banner/_index.scss 1:9  @import
    src/govuk/components/_all.scss 26:9                @import
    src/govuk/all.scss 6:9                             @import
    src/govuk/all-ie8.scss 6:9                         @import
    app/assets/scss/app-ie8.scss 1:9                   root stylesheet

Deprecations suppressed

This PR previously recreated math.div() (see implementation) but we've now suppressed warnings for:

  1. Using / for division is deprecated and will be removed in Dart Sass 2.0.0.
  2. Using / for division outside of calc() is deprecated and will be removed in Dart Sass 2.0.0.

These suppressions have been moved to:

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3192 January 20, 2023 19:42 Inactive
@colinrotherham colinrotherham force-pushed the dart-sass-fix-deprecations branch from 4018f56 to 2b26f53 Compare January 23, 2023 17:16
@colinrotherham colinrotherham force-pushed the dart-sass-prep-environment branch from 4106a2e to e077e5f Compare January 23, 2023 17:16
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3192 January 23, 2023 17:16 Inactive
@colinrotherham colinrotherham force-pushed the dart-sass-fix-deprecations branch from 2b26f53 to c8c2c57 Compare January 23, 2023 21:10
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3192 January 23, 2023 21:10 Inactive
@36degrees
Copy link
Contributor

This is a really neat solution, but I'm still hesitant about this change because:

  • we already have a solution that silences the deprecation warnings for our users which follows the approach recommended by Sass for users of libraries like ours
  • I expect that Dart Sass 2.0.0 (when the deprecated features are due to be removed) will also drop support for @import, which will force us to drop support for LibSass anyway – at which point having our own version of math.div wouldn't make any difference – though I haven't seen this documented anywhere so this is an assumption on my part
  • I also expect there will be further deprecations before 2.0 is released, and I'm just not sure it's a good use of our time trying to work around them whilst we continue to support other Sass compilers

Would be keen to get other's thoughts on this. Is there a benefit to doing this that I'm missing?

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Jan 24, 2023

Thanks @36degrees, appreciate this one's not crucial

The real good stuff like the Sass modern compilation API for 2.0 is in this PR instead:

This PR though does gives us a nice way to start using Dart Sass in spikes without hiding those warnings away. So when new deprecations arrive like "Passing a number without unit % is deprecated" we don't miss them in the terminal

Alternatively { quietDeps: true } works but we'd have to move govuk-frontend to a "dep" (dependency)

Before

Review app imports by resolving local files

@import "../../../src/govuk/all";

After

Review app imports via loadPaths + { quietDeps: true }

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

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Jan 24, 2023

This bit from the quietDeps documentation is good:

This is useful for silencing deprecation warnings that you can’t fix on your own. However, please also notify your dependencies of the deprecations so that they can get fixed as soon as possible!

If we prefer { quietDeps: true } should we try and surface warnings to ourselves another way instead?

@romaricpascal
Copy link
Member

romaricpascal commented Jan 26, 2023

Would be keen to get other's thoughts on this. Is there a benefit to doing this that I'm missing?

Have teams reported those deprecation warnings being an issue or is it more for our own peace of mind/standard that we're trying to tidy them up? If the later (either because they don't care about the flurry or warning, or ignore them with quietDeps), I think we can avoid having a division workaround in our codebase and wait for adopting the new version of Sass/the warnings being enough of an issue.

If we were to add a division workaround, though, I think it should be unit tested, as it'd be something we'd easily reach for when building further features. I originally thought it would be worth making it public as well (should we have it), but projects where the warning would appear would us a Sass version that also offer the math.div function as well for teams to use in their own code.

The implementation also looks much more complex than Bootstrap's. Are we doing something they don't (esp. the units bit and separation into two functions)?

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Jan 26, 2023

Regarding deprecation warnings, we'll have some joint squad decisions to make

Have teams reported those deprecation warnings being an issue or is it more for our own peace of mind/standard that we're trying to tidy them up?

@romaricpascal If you scroll down on this issue it's referenced fairly frequently:

Deprecation warnings

Assuming we're going ahead with Dart Sass, we have three options:

  1. Log all warnings at all times
    We'll see warnings our own Sass stylesheets but also dependencies via loadPaths

      // via `loadPaths`
      @import "govuk_frontend_toolkit/stylesheets/*";
      @import "govuk-elements-sass/public/sass/govuk-elements";
      
      // via local path resolution
      @import "../../../src/govuk/all";

    Thoughts: Lots of noisy "log spam" from things we can't fix

  2. Log warnings for our stylesheets only via { quietDeps }
    We'll see warnings for only our own Sass stylesheets

      // via local path resolution
      @import "../../../src/govuk/all";

    Thoughts: We hide the warnings that don't matter, but clearly see new deprecations

  3. Silence all warnings for all stylesheets via { quietDeps }
    By using @import "govuk-frontend/govuk/all" we can silence our own code as a "dependency"

      // via `loadPaths`
      @import "govuk-frontend/govuk/all";

    Thoughts: We'll miss new deprecations as we move towards Dart Sass 2.0.0

I've assumed we'd want 2) to reduce console spam but not hide away important messages

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3192 January 26, 2023 18:00 Inactive
@colinrotherham
Copy link
Contributor Author

colinrotherham commented Jan 26, 2023

The implementation also looks much more complex than Bootstrap's. Are we doing something they don't (esp. the units bit and separation into two functions)?

@romaricpascal I hadn't actually seen Bootstrap's version when I pushed this PR up, but we're both using the same type of long division to count the number of subtractions 😊

Things they've done differently

  1. Returning 0 early if the first number is zero
  2. Throwing when trying to divide by 0
  3. Manually rounding the last digit in 66.66666%66.66667%

They've used this little snippet for the last digit:

@if ($precision < 0 and $remainder >= $divisor * 5) {
  $result: $result + 1;
}

Things I've done differently

  1. Grabbed the remainder using % modulo operator
  2. Shifted the decimal remainder by 100 (not 10) to reduce loops
  3. Automatically letting Sass round 66.666666%66.66667%

Could definitely combine my two functions into one if preferred

Or leave it out entirely!

@colinrotherham colinrotherham force-pushed the dart-sass-prep-environment branch from e077e5f to 461ea83 Compare February 2, 2023 18:18
@colinrotherham colinrotherham force-pushed the dart-sass-fix-deprecations branch from c8c2c57 to 19fbc86 Compare February 2, 2023 18:19
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3192 February 2, 2023 18:20 Inactive
@colinrotherham colinrotherham force-pushed the dart-sass-fix-deprecations branch from 19fbc86 to d69a4de Compare February 2, 2023 20:11
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3192 February 2, 2023 20:11 Inactive
@36degrees
Copy link
Contributor

I think we should fix the '$weight: Passing a number without unit % (30) is deprecated.' warnings now, but I don't think we should be trying to reimplement math.div.

My preference would be to go with option 2, but also see if there's a way to filter out the 'known' warnings – so we don't see warnings for math.div for example, but if we upgrade dart-sass and a new deprecation warning is introduced, we'd see it (and then either fix it or silence the new warning too).

@colinrotherham colinrotherham force-pushed the dart-sass-prep-environment branch from 461ea83 to 1ae2bf6 Compare February 6, 2023 18:09
@colinrotherham colinrotherham force-pushed the dart-sass-fix-deprecations branch from 07dbd73 to 4a8ac8f Compare February 14, 2023 09:06
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3192 February 14, 2023 09:07 Inactive
@colinrotherham colinrotherham force-pushed the dart-sass-fix-deprecations branch from 4a8ac8f to b6208ef Compare February 14, 2023 09:20
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3192 February 14, 2023 09:20 Inactive
@@ -66,6 +66,9 @@ export async function compileStylesheet ([modulePath, { srcPath, destPath }]) {
file: moduleSrcPath,
outFile: moduleDestPath,

// Turn off dependency warnings
quietDeps: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whilst this PR fixes govuk-frontend deprecation warnings we add quietDeps: true to ignore:

govuk_frontend_toolkit
govuk-elements-sass

See https://frontend.design-system.service.gov.uk/importing-css-assets-and-javascript/#silence-deprecation-warnings-from-dependencies-in-dart-sass

@colinrotherham colinrotherham changed the title Fix Dart Sass deprecation warnings Fix or suppress Dart Sass deprecation warnings Feb 14, 2023
@colinrotherham
Copy link
Contributor Author

I think we should fix the '$weight: Passing a number without unit % (30) is deprecated.' warnings now, but I don't think we should be trying to reimplement math.div.

@36degrees Updated the description to say we've fixed these two:

  1. $string: red is not a string.
  2. $weight: Passing a number without unit % (30) is deprecated.

My preference would be to go with option 2, but also see if there's a way to filter out the 'known' warnings – so we don't see warnings for math.div for example, but if we upgrade dart-sass and a new deprecation warning is introduced, we'd see it (and then either fix it or silence the new warning too).

Added your suggestion in this PR:

We needed access to the Dart Sass modern compilation API logger property so depends on:

@colinrotherham colinrotherham force-pushed the dart-sass-fix-deprecations branch from b6208ef to 780a8d0 Compare February 14, 2023 11:34
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3192 February 14, 2023 11:34 Inactive
@colinrotherham colinrotherham force-pushed the dart-sass-fix-deprecations branch from 780a8d0 to f90c5ce Compare February 15, 2023 15:48
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3192 February 15, 2023 15:49 Inactive
Base automatically changed from standalone-sass-task to main February 15, 2023 15:56
Copy link
Contributor

@domoscargin domoscargin left a comment

Choose a reason for hiding this comment

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

These all feel like non-controversial changes now (having swooped in after everybody else dealt with the nitty gritty).

I'm happy to approve, but appreciate @36degrees and @romaricpascal might want to weigh in?

Dart Sass will throw when trying to `quote()` colour identifiers as it no longer treats them as unquoted strings

See “Heads up!” box on Strings (Unquoted) docs:
https://sass-lang.com/documentation/values/strings#unquoted
For example we can’t fix “Using / for division outside of calc() is deprecated” in:

```
govuk_frontend_toolkit
govuk-elements-sass
```

See https://frontend.design-system.service.gov.uk/importing-css-assets-and-javascript/#silence-deprecation-warnings-from-dependencies-in-dart-sass
@colinrotherham colinrotherham force-pushed the dart-sass-fix-deprecations branch from f90c5ce to 3098cca Compare February 17, 2023 17:15
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3192 February 17, 2023 17:15 Inactive
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Changes look sound to me! 🚀

@colinrotherham colinrotherham merged commit 143ab04 into main Feb 21, 2023
@colinrotherham colinrotherham deleted the dart-sass-fix-deprecations branch February 21, 2023 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

6 participants