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 Review app Sass compile after error #4239

Merged
merged 4 commits into from Sep 21, 2023
Merged

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Sep 20, 2023

This PR fixes Gulp "Did you forget to signal async completion?" when Sass compilation errors occur in:

The issue

Spot my missing await in 8f74d02 🤦‍♂️

Error logging

I've made sure all "watch + reload" tasks use Gulp's new PluginError() for consistent output, as currently:

  1. Sass tasks via scripts.compile() throw new PluginError()
  2. Rollup tasks via styles.compile() throw new PluginError()
  3. npm tasks via npm.script() log console.error(error.message) without error

But Gulp is now aware of npm task errors too, without interrupting watch tasks:

ESLint errors now known to Gulp

This ensures Gulp can still catch errors without “Did you forget to signal async completion?”
@colinrotherham colinrotherham added the 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) label Sep 20, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4239 September 20, 2023 18:33 Inactive
@colinrotherham colinrotherham linked an issue Sep 20, 2023 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Sep 20, 2023

📋 Stats

File sizes

File Size
dist/govuk-frontend-4.6.0.min.css 118.07 KiB
dist/govuk-frontend-4.6.0.min.js 47.1 KiB
dist/govuk-frontend-ie8-4.6.0.min.css 79.27 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 76.42 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 71.79 KiB
packages/govuk-frontend/dist/govuk/all.mjs 3.84 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 37.16 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.83 KiB

Modules

File Size
all.mjs 67.89 KiB
components/accordion/accordion.mjs 21.9 KiB
components/button/button.mjs 4.71 KiB
components/character-count/character-count.mjs 21.64 KiB
components/checkboxes/checkboxes.mjs 5.69 KiB
components/error-summary/error-summary.mjs 6.01 KiB
components/exit-this-page/exit-this-page.mjs 16.6 KiB
components/header/header.mjs 3.34 KiB
components/notification-banner/notification-banner.mjs 4.54 KiB
components/radios/radios.mjs 4.68 KiB
components/skip-link/skip-link.mjs 3.73 KiB
components/tabs/tabs.mjs 9.07 KiB

View stats and visualisations on the review app


Action run for 089b280

Unlike Sass and npm scripts, Rollup includes error properties from Babel such as:

Details:
    code: PLUGIN_ERROR
    reasonCode: UnexpectedToken
    loc: [object Object]
    pos: 13330
    pluginCode: BABEL_PARSE_ERROR
    hook: transform
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.

Whoops! So easy to miss these await. Thanks for the little tidy-ups around as well :D

@colinrotherham
Copy link
Contributor Author

Thanks @romaricpascal

Just need to confirm those error codes etc in Windows before merging

Copy link
Member

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

🙌🏻

The previous commit lets Gulp hande `new PluginError()` versus logging to console, so we now need to skip typical “watch + restart” errors seen during development
@colinrotherham
Copy link
Contributor Author

To avoid unnecessary error logging on Windows I've silenced { code: 3221225786 } as shown:

Don't want to make Ctrl + C too noisy on Windows if we can avoid it there too

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4239 September 21, 2023 12:54 Inactive
@colinrotherham colinrotherham merged commit 10b5b69 into main Sep 21, 2023
44 checks passed
@colinrotherham colinrotherham deleted the styles-watch-error branch September 21, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation) sass / css
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The review app stops compiling Sass after encountering an error
4 participants