Support sass errors in error pages #2287

BenSurgisonGDS · 2023-08-03T14:59:05Z

See: Sass errors are not obvious as the kit uses previously compiled sass in .tmp

Support restarting nodemon when sass files are in error or not in error
Display SASS error page correctly including the line where the error occurs
Add new SASS error acceptance test
Fix acceptance tests affected by this change

nataliecarey · 2023-08-15T13:56:41Z

I've just tried to run this, the following attempt lead to the kit failing to start (failing to show the error in the browser):

npx govuk-prototype-kit create --version=alphagov/govuk-prototype-kit#2170-sass-errors-are-not-obvious-as-the-kit-uses-previously-compiled-sass-in-tmp broken-scss-kit
cd broken-scss-kit
echo "body {" >> app/assets/sass/application.scss
npm run dev

Creating the error after the kit has started leads to the error appears properly in the browser as expected but the same does not happen if the error existed before starting the kit.

@BenSurgisonGDS could you take a look at that case please?

BenSurgisonGDS · 2023-08-15T16:08:11Z

I've just tried to run this, the following attempt lead to the kit failing to start (failing to show the error in the browser):

npx govuk-prototype-kit create --version=alphagov/govuk-prototype-kit#2170-sass-errors-are-not-obvious-as-the-kit-uses-previously-compiled-sass-in-tmp broken-scss-kit

cd broken-scss-kit

echo "body {" >> app/assets/sass/application.scss

npm run dev

Creating the error after the kit has started leads to the error appears properly in the browser as expected but the same does not happen if the error existed before starting the kit.

@BenSurgisonGDS could you take a look at that case please?

I have made some changes to fix the above issue. This has included how the sass for the error page is compiled and where it is sourced from.

nataliecarey · 2023-08-15T16:59:42Z

...prototype-kit-plugin-tests/handle-plugin-update-when-a-dependency-is-now-required.cypress.js

@@ -10,7 +10,7 @@ const dependencyPluginName = 'GOV.UK Frontend'
 describe('Handle a plugin update', () => {
  after(restoreStarterFiles)

-  it('when a dependency is now required', () => {
+  it.skip('when a dependency is now required', () => {


Is this one supposed to be skipped? It looks like maybe it was meant to be temporary.

This needs to either be deleted or revisited as the test no longer works as it did previously. This is because the older version of common-templates does not specify in it's config that it depends on govuk-frontend but it does because the sass fails to compile leading to the sass compilation error and so we can no longer (at the moment) have a test where a plugin depends upon another plugin when it is updated.

For now, I thought it best to skip until we have an example we can use.

nataliecarey · 2023-08-15T17:01:58Z

lib/assets/sass/prototype.scss

-  }
-}
+// SASS that is user defined
+@import ".tmp/sass/user/application";


Can you add an empty line at the end of the file please? (there's a setting to do this automatically in WebStorm)

nataliecarey · 2023-08-15T17:17:11Z

lib/utils/errorModel.js

@@ -188,7 +209,7 @@ function getErrorDetails (cwd, error) {
 const getErrorModel = function (error) {
  flagError(error)
  const viewObject = {
-    errorStack: error.stack,
+    errorStack: unstyle(error.stack),


If it's only SASS errors that need unstyling (so far) I think we should move this into sassMatcher().

I would argue that it doesn't hurt where it is and it covers us for if we ever come across any other errors that are styled.

I'd rather keep it more contained - otherwise things like this tend to be left in through fear or what would happen if they're removed.

I've moved the unstyling of the error stack into the sass matcher as requested but then had to add some extra scafolding to cater for the fact that we don't return the stack from each matcher. To be honest I like the cleaner version we had before, but I've made the change as you asked.

nataliecarey · 2023-08-15T17:32:41Z

I've just tested a few things manually and found that breaking app/config.json now leads to the kit not starting. Steps to reproduce:

npx govuk-prototype-kit create --version=alphagov/govuk-prototype-kit#2170-sass-errors-are-not-obvious-as-the-kit-uses-previously-compiled-sass-in-tmp broken-scss-kit
cd broken-config-kit
echo "body {" >> app/config.json
npm run dev

To see how it should behave (which is still not technically correct but it's the existing behaviour) just replace step 1 with:

npx govuk-prototype-kit create broken-scss-kit

It would be good to put a test in for this, but up to you whether that's done as part of this ticket or not.

BenSurgisonGDS

It's looks good

BenSurgisonGDS linked an issue Aug 3, 2023 that may be closed by this pull request

Sass errors are not obvious as the kit uses previously compiled sass in .tmp #2170

Closed

BenSurgisonGDS force-pushed the 2170-sass-errors-are-not-obvious-as-the-kit-uses-previously-compiled-sass-in-tmp branch 6 times, most recently from 64cbf22 to edce060 Compare August 11, 2023 08:22

BenSurgisonGDS force-pushed the 2170-sass-errors-are-not-obvious-as-the-kit-uses-previously-compiled-sass-in-tmp branch from 83d5b16 to c409654 Compare August 11, 2023 11:48

Support sass errors in error pages

648b94f

BenSurgisonGDS force-pushed the 2170-sass-errors-are-not-obvious-as-the-kit-uses-previously-compiled-sass-in-tmp branch from c409654 to 648b94f Compare August 11, 2023 12:07

BenSurgisonGDS marked this pull request as ready for review August 11, 2023 12:14

BenSurgisonGDS requested a review from nataliecarey August 11, 2023 12:34

Fix sass error when creating the .tmp sass first time

c8104e7

nataliecarey reviewed Aug 15, 2023

View reviewed changes

Add line break

e8e2d92

BenSurgisonGDS and others added 3 commits August 15, 2023 19:11

Move unstyling the error stack into the sass matcher

cea2410

Added error stack to tests.

65b2532

Adding backups for Sass functions to allow update testing.

e2f8107

nataliecarey approved these changes Aug 16, 2023

View reviewed changes

BenSurgisonGDS requested a review from nataliecarey August 16, 2023 13:52

BenSurgisonGDS commented Aug 16, 2023

View reviewed changes

BenSurgisonGDS requested a review from oli-rose28 August 16, 2023 14:28

oli-rose28 approved these changes Aug 16, 2023

View reviewed changes

BenSurgisonGDS merged commit e69cf0f into main Aug 16, 2023
27 checks passed

BenSurgisonGDS deleted the 2170-sass-errors-are-not-obvious-as-the-kit-uses-previously-compiled-sass-in-tmp branch August 16, 2023 14:28

BenSurgisonGDS mentioned this pull request Aug 17, 2023

Preparing for release 13.12.1 (automated). #2300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support sass errors in error pages #2287

Support sass errors in error pages #2287

BenSurgisonGDS commented Aug 3, 2023 •

edited

Loading

nataliecarey commented Aug 15, 2023

BenSurgisonGDS commented Aug 15, 2023

nataliecarey Aug 15, 2023

BenSurgisonGDS Aug 15, 2023

BenSurgisonGDS Aug 15, 2023

nataliecarey Aug 15, 2023

BenSurgisonGDS Aug 15, 2023

nataliecarey Aug 15, 2023

BenSurgisonGDS Aug 15, 2023

nataliecarey Aug 15, 2023

BenSurgisonGDS Aug 15, 2023

BenSurgisonGDS Aug 15, 2023

nataliecarey commented Aug 15, 2023

BenSurgisonGDS left a comment

Support sass errors in error pages #2287

Support sass errors in error pages #2287

Conversation

BenSurgisonGDS commented Aug 3, 2023 • edited Loading

nataliecarey commented Aug 15, 2023

BenSurgisonGDS commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nataliecarey commented Aug 15, 2023

BenSurgisonGDS left a comment

Choose a reason for hiding this comment

BenSurgisonGDS commented Aug 3, 2023 •

edited

Loading