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

Enable ESLint stylistic typed linting in source code #4106

Merged
merged 5 commits into from
Nov 8, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Aug 18, 2023

This PR enables ESLint stylistic typed linting

It highlights opportunities to replace older JavaScript with new, but also uses TypeScript { "strict": true } to add warnings when our GOV.UK Frontend source code is too loose or unnecessarily defensive

As part of #4323 we've already fixed:

  • Class fields with incorrect null default
  • Selector null return values not handled
  • Timeout and interval IDs cleared when null
  • Confg allows null as typeof value === 'object'

But this PR includes a few more:

  1. Potentially null attribute NamedNodeMap values
  2. Potentially null element .parentNode values
  3. Potentially null element .textContent values

Whilst quite pedantic for internal code, it's a huge safety net as we clarify our public API and catch issues like:

Stylistic typed linting

This change lets ESLint use types to reduce unnecessary code with suggestions to use modern syntax

Unnecessary conditional

Strict mode checks (existing)

Here's a list of the compiler options we already enabled in the past:

Strict mode checks (new)

Here's a list of the compiler options enabled in this PR:

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 August 18, 2023 14:37 Inactive
@romaricpascal
Copy link
Member

Cheers! Not sure it's one we'll merge straight away, but it's good to have the steps to enable it. I'm also wondering if us throwing (rather than returning early) during initialisation will not remove a couple of these "watch out it may be null" warnings, which would be nice and may make the list of things to fix less daunting 😊

@colinrotherham colinrotherham requested a review from a team as a code owner August 23, 2023 18:18
@colinrotherham colinrotherham changed the base branch from main to tsconfig-src-dev August 23, 2023 18:18
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 August 23, 2023 18:18 Inactive
@colinrotherham colinrotherham force-pushed the tsconfig-src-dev branch 2 times, most recently from 7b509e6 to 3d7f5fb Compare August 23, 2023 18:49
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 August 23, 2023 18:49 Inactive
@github-actions
Copy link

github-actions bot commented Aug 23, 2023

📋 Stats

File sizes

File Size
dist/govuk-frontend-5.0.0-beta.1.min.css 114 KiB
dist/govuk-frontend-5.0.0-beta.1.min.js 37.93 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 77.67 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 72.97 KiB
packages/govuk-frontend/dist/govuk/all.mjs 3.86 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 113.53 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 38.14 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.38 KiB

Modules

File Size
all.mjs 69.3 KiB
components/accordion/accordion.mjs 21.55 KiB
components/button/button.mjs 4.58 KiB
components/character-count/character-count.mjs 21.13 KiB
components/checkboxes/checkboxes.mjs 5.71 KiB
components/error-summary/error-summary.mjs 5.97 KiB
components/exit-this-page/exit-this-page.mjs 15.97 KiB
components/header/header.mjs 3.79 KiB
components/notification-banner/notification-banner.mjs 4.42 KiB
components/radios/radios.mjs 4.71 KiB
components/skip-link/skip-link.mjs 3.69 KiB
components/tabs/tabs.mjs 9.55 KiB

View stats and visualisations on the review app


Action run for 331d7f0

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 August 23, 2023 19:14 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 August 23, 2023 20:51 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 August 24, 2023 15:19 Inactive
Base automatically changed from tsconfig-src-dev to main August 30, 2023 13:00
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 August 31, 2023 17:25 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 September 13, 2023 11:02 Inactive
@domoscargin
Copy link
Contributor

Just rebasing to keep this branch up to date

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 September 21, 2023 12:20 Inactive
@colinrotherham
Copy link
Contributor Author

Just rebasing to keep this branch up to date

Thanks @domoscargin

Shame GitHub limits the number of annotations on the Summary (scroll down)

But you'll see them all in the logs

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 October 24, 2023 08:39 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 October 24, 2023 08:58 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 October 27, 2023 12:36 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 October 27, 2023 12:49 Inactive
We widely use the shorthand form already where the return value isn’t important
Built source files only, excluding tasks and tests
We have another PR open to discuss these features: #4286

Some ES2015+ features already have browser support or are enabled by Babel transforms
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4106 November 8, 2023 12:12 Inactive
Copy link

github-actions bot commented Nov 8, 2023

JavaScript changes to dist

diff --git a/dist/govuk-frontend-5.0.0-beta.0.min.js b/dist/govuk-frontend-5.0.0-beta.1.min.js
index 45cebefc1..5a255dbcb 100644
--- a/dist/govuk-frontend-5.0.0-beta.0.min.js
+++ b/dist/govuk-frontend-5.0.0-beta.1.min.js
@@ -1,4 +1,4 @@
-const version = "5.0.0-beta.0";
+const version = "5.0.0-beta.1";
 
 function mergeConfigs(...t) {
     function flattenObject(t) {
@@ -35,7 +35,7 @@ function getFragmentFromUrl(t) {
 }
 
 function isSupported(t = document.body) {
-    return t.classList.contains("govuk-frontend-supported")
+    return !!t && t.classList.contains("govuk-frontend-supported")
 }
 
 function normaliseString(t) {
@@ -55,8 +55,8 @@ class GOVUKFrontendError extends Error {
     }
 }
 class SupportError extends GOVUKFrontendError {
-    constructor() {
-        super("GOV.UK Frontend is not supported in this browser"), this.name = "SupportError"
+    constructor(t = document.body) {
+        super(t ? "GOV.UK Frontend is not supported in this browser" : 'GOV.UK Frontend initialised without `<script type="module">`'), this.name = "SupportError"
     }
 }
 class ConfigError extends GOVUKFrontendError {
@@ -971,4 +971,4 @@ export {
     Tabs,
     initAll,
     version
-}; //# sourceMappingURL=govuk-frontend-5.0.0-beta.0.min.js.map
\ No newline at end of file
+}; //# sourceMappingURL=govuk-frontend-5.0.0-beta.1.min.js.map
\ No newline at end of file

Action run for 331d7f0

Copy link

github-actions bot commented Nov 8, 2023

Other changes to dist

diff --git a/dist/VERSION.txt b/dist/VERSION.txt
index 5183a25c3..9e25fc0ae 100644
--- a/dist/VERSION.txt
+++ b/dist/VERSION.txt
@@ -1 +1 @@
-5.0.0-beta.0
+5.0.0-beta.1

Action run for 331d7f0

Copy link

github-actions bot commented Nov 8, 2023

Stylesheets changes to dist

diff --git a/dist/govuk-frontend-5.0.0-beta.0.min.css b/dist/govuk-frontend-5.0.0-beta.1.min.css
index a25f51b2e..cd71490d1 100644
--- a/dist/govuk-frontend-5.0.0-beta.0.min.css
+++ b/dist/govuk-frontend-5.0.0-beta.1.min.css
@@ -1,7 +1,7 @@
 @charset "UTF-8";
 
 :root {
-    --govuk-frontend-version: "5.0.0-beta.0"
+    --govuk-frontend-version: "5.0.0-beta.1"
 }
 
 .govuk-link {
@@ -7692,4 +7692,4 @@ screen and (-ms-high-contrast:active) {
     }
 }
 
-/*# sourceMappingURL=govuk-frontend-5.0.0-beta.0.min.css.map */
\ No newline at end of file
+/*# sourceMappingURL=govuk-frontend-5.0.0-beta.1.min.css.map */
\ No newline at end of file

Action run for 331d7f0

@colinrotherham colinrotherham changed the base branch from syntax-enabled to main November 8, 2023 12:14
@colinrotherham
Copy link
Contributor Author

Following dev catch-up this week we agreed to get this PR merged to test things out

I've assumed we're undecided on ES2015+ syntax suggestions from #4286 so they're turned off in 331d7f0

@domoscargin @romaricpascal Mind giving this the green tick please?

Thanks 🙌

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.

Makes sense, let's give it a go.

@colinrotherham colinrotherham merged commit 136b81b into main Nov 8, 2023
47 checks passed
@colinrotherham colinrotherham deleted the strict-mode branch November 8, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants