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

Add warning suppression functionality #2911

Merged
merged 3 commits into from
Oct 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,18 @@ This was added in pull requests:
- [#2818: Add support for localisation via data-* attributes to Accordion component](https://github.com/alphagov/govuk-frontend/pull/2818)
- [#2826: Add support for localisation via JavaScript configuration to Accordion component](https://github.com/alphagov/govuk-frontend/pull/2826)

#### Suppress deprecation warnings

You can now suppress warnings from deprecations within GOV.UK Frontend by updating the `$govuk-suppressed-warnings` map in sass. Every deprecation warning will now include a warning "key" which you can use in the following code, placed at the root of your sass project:

```scss
$govuk-suppressed-warnings: (
deprecated-feature
);
```

This was added in [#2911 Add warning suppression functionality](https://github.com/alphagov/govuk-frontend/pull/2911)

### Recommended changes

#### Remove `aria-labelledby`, remove `id="error-summary-title"` from title and move `role="alert"` to child container on the error summary component
Expand Down
18 changes: 13 additions & 5 deletions docs/contributing/managing-change.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ In the annotation description include:
- the suggested alternative, if there is one
- a link to the GitHub issue for its removal

If possible, update the mixin or function to output a warning (using `@warn`).
If possible, update the mixin or function to output a warning using the `_warning` mixin (see section below on allowing users to suppress warnings).

For example:

Expand All @@ -67,7 +67,7 @@ For example:
/// @deprecated Use govuk-multiply(number, 2) instead.
/// See https://github.com/alphagov/govuk-frontend/issues/1234
@function govuk-double($number) {
@warn "govuk-double($number) is deprecated. Use govuk-multiply($number, 2) instead.";
@include _warning("double", "govuk-double($number) is deprecated. Use govuk-multiply($number, 2) instead.");
@return govuk-multiply($number, 2);
}
```
Expand All @@ -86,7 +86,7 @@ If possible, update the mixin or function to maintain the existing functionality
/// @param {Boolean} $rightAngle Deprecated. Use $angle: 90 instead.
@mixin govuk-reticulate-splines($spline, $angle: 180, $rightAngle: false) {
@if ($rightAngle != false) {
@warn "Passing $rightAngle to govuk-reticulate-splines is deprecated. Pass $angle: 90 instead.";
@include _warning("right-angle", "Passing $rightAngle to govuk-reticulate-splines is deprecated. Pass $angle: 90 instead.");

$angle: 90;
}
Expand Down Expand Up @@ -123,7 +123,7 @@ For example:
/// @alias the-new-name
/// @deprecated Use the-new-name($foo) instead.
@function the-old-name($foo) {
@warn "the-old-name is deprecated. Use the-new-name instead.";
@include _warning("the-old-name", "the-old-name is deprecated. Use the-new-name instead.");
@return the-new-name($foo);
}

Expand Down Expand Up @@ -151,7 +151,7 @@ Add 'Deprecated.' to the description for the parameter.
/// @param {String} $spilne Deprecated. Use $spline instead.
@function govuk-reticulate-splines($spline, $spilne: false) {
@if ($spilne != false) {
@warn "Passing $spilne to govuk-reticulate-splines is deprecated. Pass $spline instead.";
@include _warning("spilne", "Passing $spilne to govuk-reticulate-splines is deprecated. Pass $spline instead.");

$spline: $spilne;
}
Expand All @@ -171,3 +171,11 @@ Keep the old name in the selector list, and mark it as deprecated.
foo: bar;
}
```

### The `_warning` mixin and allowing users to suppress warnings
owenatgov marked this conversation as resolved.
Show resolved Hide resolved

In the above examples we've used `@include _warning(...)` instead of the native sass `@warn` at-rule. We use this instead of `@warn` because it gives users the option to suppress deprecation warnings by interacting with the `$govuk-suppressed-warnings` map.

You can read more about how `$govuk-suppressed-warnings` and `_warning` work by reading their respective sassdocs in `/src/govuk/settings/warnings.scss`.

We make this option available for users because they can not always action deprecation warnings or upgrade their codebase beyond a specific version of GOV.UK Frontend. For example, a legacy codebase that does not have the resource to upgrade to the latest breaking change where a deprecated feature will be removed. This feature allows those users to continue to operate their codebase without having to repeatedly see non-actionable deprecation warnings in their testing.
6 changes: 4 additions & 2 deletions src/govuk/helpers/colour.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ describe('@function govuk-colour', () => {
describe('when $govuk-use-legacy-palette is true', () => {
beforeEach(() => {
sassBootstrap = `
@import "settings/warnings";
$govuk-use-legacy-palette: true;
${sassBootstrap}
`
Expand Down Expand Up @@ -138,8 +139,9 @@ describe('@function govuk-colour', () => {
// argument, which should be the deprecation notice
return expect(mockWarnFunction.mock.calls[0][0].getValue())
.toEqual(
'$govuk-use-legacy-palette is deprecated. ' +
'Only the modern colour palette will be supported from v5.0'
'$govuk-use-legacy-palette is deprecated. Only the modern colour ' +
'palette will be supported from v5.0. To silence this warning, ' +
'update $govuk-suppressed-warnings with key: "legacy-palette"'
)
})
})
Expand Down
22 changes: 16 additions & 6 deletions src/govuk/helpers/typography.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const sassConfig = {
}
}

const sassBootstrap = `
let sassBootstrap = `
$govuk-breakpoints: (
desktop: 30em
);
Expand Down Expand Up @@ -335,6 +335,13 @@ describe('@mixin govuk-typography-responsive', () => {
})

describe('when $govuk-typography-use-rem is disabled', () => {
beforeEach(() => {
sassBootstrap = `
@import "settings/warnings";
${sassBootstrap}
`
})

it('outputs CSS with suitable media queries', async () => {
const sass = `
$govuk-typography-use-rem: false;
Expand Down Expand Up @@ -411,8 +418,10 @@ describe('@mixin govuk-typography-responsive', () => {
// deprecation notice
return expect(mockWarnFunction.mock.calls.at(-1)[0].getValue())
.toEqual(
'$govuk-typography-use-rem is deprecated. ' +
'From version 5.0, GOV.UK Frontend will not support disabling rem font sizes'
'$govuk-typography-use-rem is deprecated. From version 5.0, ' +
'GOV.UK Frontend will not support disabling rem font sizes. To ' +
'silence this warning, update $govuk-suppressed-warnings with ' +
'key: "allow-not-using-rem"'
)
})
})
Expand Down Expand Up @@ -609,9 +618,10 @@ describe('$govuk-font-family-tabular value is specified', () => {
// deprecation notice
return expect(mockWarnFunction.mock.calls.at(-1)[0].getValue())
.toEqual(
'$govuk-font-family-tabular is deprecated. ' +
'From version 5.0, GOV.UK Frontend will not support using a separate ' +
'font-face for tabular numbers'
'$govuk-font-family-tabular is deprecated. From version 5.0, ' +
'GOV.UK Frontend will not support using a separate font-face for ' +
'tabular numbers. To silence this warning, update ' +
'$govuk-suppressed-warnings with key: "tabular-font-face"'
)
})
})
Expand Down
1 change: 1 addition & 0 deletions src/govuk/settings/_all.scss
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

@import "assets";

@import "warnings";
@import "compatibility";
@import "global-styles";
@import "ie8";
Expand Down
4 changes: 2 additions & 2 deletions src/govuk/settings/_colours-palette.scss
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ $govuk-use-legacy-palette: if(
$govuk-compatibility-govukfrontendtoolkit == false and
$govuk-compatibility-govuktemplate == false and
$govuk-compatibility-govukelements == false {
@warn "$govuk-use-legacy-palette is deprecated. " +
"Only the modern colour palette will be supported from v5.0";
@include _warning(legacy-palette, "$govuk-use-legacy-palette is deprecated. " +
"Only the modern colour palette will be supported from v5.0.");
}

/// Modern colour palette
Expand Down
12 changes: 6 additions & 6 deletions src/govuk/settings/_typography-font.scss
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ $govuk-use-legacy-font: if(
$govuk-compatibility-govukfrontendtoolkit == false and
$govuk-compatibility-govuktemplate == false and
$govuk-compatibility-govukelements == false {
@warn "$govuk-use-legacy-font is deprecated. " +
"From version 5.0, GOV.UK Frontend will only support the included version " +
"of GDS Transport";
@include _warning(legacy-font, "$govuk-use-legacy-font is deprecated. " +
"From version 5.0, GOV.UK Frontend will only support the included version " +
"of GDS Transport.");
}

// =========================================================
Expand Down Expand Up @@ -68,9 +68,9 @@ $govuk-font-family-tabular: if(
// Only show the deprecation warning if user is setting $govuk-font-family-tabular
// manually instead of automatically via $govuk-use-legacy-font
@if $govuk-font-family-tabular != false and $govuk-use-legacy-font == false {
@warn "$govuk-font-family-tabular is deprecated. " +
"From version 5.0, GOV.UK Frontend will not support using a separate " +
"font-face for tabular numbers";
@include _warning(tabular-font-face, "$govuk-font-family-tabular is deprecated. " +
"From version 5.0, GOV.UK Frontend will not support using a separate " +
"font-face for tabular numbers.");
}

/// Font families to use for print media
Expand Down
4 changes: 2 additions & 2 deletions src/govuk/settings/_typography-responsive.scss
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ $govuk-typography-use-rem: if(
$govuk-compatibility-govukfrontendtoolkit == false and
$govuk-compatibility-govuktemplate == false and
$govuk-compatibility-govukelements == false {
@warn "$govuk-typography-use-rem is deprecated. " +
"From version 5.0, GOV.UK Frontend will not support disabling rem font sizes";
@include _warning(allow-not-using-rem, "$govuk-typography-use-rem is deprecated. " +
"From version 5.0, GOV.UK Frontend will not support disabling rem font sizes.");
}

/// Root font size
Expand Down
53 changes: 53 additions & 0 deletions src/govuk/settings/_warnings.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
////
/// @group settings/warnings
////

/// Suppressed warnings map
///
/// This map is used to determine which deprecation warnings to **not** show
/// to users when compiling sass. This is in place for codebases that do not
/// have the necessary capacity to upgrade and remove the deprecation,
/// particularly if the deprecation is significant. For example, removal of
/// compatibility with legacy libraries such as govuk_elements.
///
/// You can add to this map and define which warnings to suppress by appending to
/// it using the warning key, found in the warning message. For example:
///
/// @example scss:
/// // warning message:
/// // $foobar is no longer supported. To silence this warning, update
/// // $govuk-suppressed-warnings with key: "foobar"
/// $govuk-suppressed-warnings: (
/// foobar
/// );
///
/// @type List
/// @access public

$govuk-suppressed-warnings: () !default;

/// Warnings
///
/// Acts as a wrapper for the built in `@warn` sass function
///
/// We use this instead of using `@warn` for 3 reasons:
///
/// - To check if a warning is being suppressed through `$govuk-suppressed-warnings`,
/// in which case we don't call `@warn` and printing the warning to the user
/// - To format the passed warning `$message` with the warning key at the end
/// - To prevent duplicate warnings by adding the passed `$key` to
/// `$govuk-suppressed-warnings` after `@warn` is called to ensure it only runs
/// once per sass compilation
///
/// @param {String} $key - The key to be checked against `$govuk-suppressed-warnings`
/// and then passed to it to prevent multiple of the same warning.
/// @param {String} $message - The message to use when calling `@warn`
/// @access private

@mixin _warning($key, $message) {
@if not index($govuk-suppressed-warnings, $key) {
@warn $message + " To silence this warning, update $govuk-suppressed-warnings " +
"with key: \"#{$key}\"";
$govuk-suppressed-warnings: append($govuk-suppressed-warnings, $key) !global;
}
}
63 changes: 63 additions & 0 deletions src/govuk/settings/warnings.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
const sass = require('node-sass')
const { renderSass } = require('../../../lib/jest-helpers')

// Create a mock warn function that we can use to override the native @warn
// function, that we can make assertions about post-render.
const mockWarnFunction = jest.fn()
.mockReturnValue(sass.NULL)

const sassConfig = {
outputStyle: 'compressed',
functions: {
'@warn': mockWarnFunction
}
}

describe('Warnings mixin', () => {
owenatgov marked this conversation as resolved.
Show resolved Hide resolved
const sassBootstrap = '@import "settings/warnings";'

afterEach(() => {
jest.clearAllMocks()
})

it('Fires a @warn with the message plus the key suffix text', async () => {
const sass = `
${sassBootstrap}
@include _warning('test', 'This is a warning.');`

await renderSass({ data: sass, ...sassConfig }).then(() => {
// Expect our mocked @warn function to have been called once with a single
// argument, which should be the test message
return expect(mockWarnFunction.mock.calls[0][0].getValue())
.toEqual(
'This is a warning. To silence this warning, update ' +
'$govuk-suppressed-warnings with key: "test"'
)
})
})

it('Only fires one @warn per warning key', async () => {
const sass = `
${sassBootstrap}
@include _warning('test', 'This is a warning.');
@include _warning('test', 'This is a warning.');`
owenatgov marked this conversation as resolved.
Show resolved Hide resolved

await renderSass({ data: sass, ...sassConfig }).then(() => {
// Expect our mocked @warn function to have been called once with a single
// argument, which should be the test message
return expect(mockWarnFunction.mock.calls.length).toEqual(1)
})
})

it('Does not fire a @warn if the key is already in $govuk-suppressed-warnings', async () => {
const sass = `
${sassBootstrap}

$govuk-suppressed-warnings: append($govuk-suppressed-warnings, 'test');
@include _warning('test', 'This is a warning.');`

await renderSass({ data: sass, ...sassConfig }).then(() => {
return expect(mockWarnFunction).not.toHaveBeenCalled()
})
})
})
4 changes: 2 additions & 2 deletions src/govuk/tools/_compatibility.scss
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@
/// suite of tools and settings

@mixin govuk-compatibility($product) {
@warn "govuk-compatibility is deprecated. " +
"From version 5.0, GOV.UK Frontend will not support compatibility mode";
@include _warning(compatibility-helper, "govuk-compatibility is deprecated. " +
"From version 5.0, GOV.UK Frontend will not support compatibility mode.");
@include _govuk-compatibility($product) {
@content;
}
Expand Down
11 changes: 10 additions & 1 deletion src/govuk/tools/compatibility.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,13 @@ const sassConfig = {
}

describe('@mixin govuk-compatibility', () => {
// Import the warning mixin
const warningsImport = '@import "settings/warnings";'

it('does not output if the app is not marked as included', async () => {
const sass = `
${warningsImport}

$_govuk-compatibility: (existing_app: false);

@import "tools/compatibility";
Expand All @@ -33,6 +38,7 @@ describe('@mixin govuk-compatibility', () => {

it('outputs if the app is not marked as included', async () => {
const sass = `
${warningsImport}
$_govuk-compatibility: (existing_app: true);

@import "tools/compatibility";
Expand All @@ -50,6 +56,7 @@ describe('@mixin govuk-compatibility', () => {

it('throws an exception if the app is not recognised', async () => {
const sass = `
${warningsImport}
$_govuk-compatibility: (existing_app: true);

@import "tools/compatibility";
Expand All @@ -67,6 +74,7 @@ describe('@mixin govuk-compatibility', () => {

it('outputs a deprecation warning when called', async () => {
const sass = `
${warningsImport}
$_govuk-compatibility: (existing_app: true);

@import "tools/compatibility";
Expand All @@ -83,7 +91,8 @@ describe('@mixin govuk-compatibility', () => {
return expect(mockWarnFunction.mock.calls[0][0].getValue())
.toEqual(
'govuk-compatibility is deprecated. From version 5.0, GOV.UK Frontend ' +
'will not support compatibility mode'
'will not support compatibility mode. To silence this warning, ' +
'update $govuk-suppressed-warnings with key: "compatibility-helper"'
)
})
})
Expand Down
1 change: 1 addition & 0 deletions tasks/sassdoc.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ function buildSassdocs () {
'settings/media-queries': 'Settings / Media Queries',
'settings/spacing': 'Settings / Spacing',
'settings/typography': 'Settings / Typography',
'settings/warnings': 'Settings / Warnings',
tools: 'Tools',
helpers: 'Helpers',
overrides: 'Overrides',
Expand Down