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 (optional) type declaration checks in app, src #3104

Merged
merged 9 commits into from
Feb 22, 2023
Merged

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Dec 15, 2022

This PR adds a tsconfig.json so the TypeScript compiler can check our code and closes:

What does this mean for us?

  1. Checks we're calling library code as intended
  2. Checks we're calling DOM methods as intended
  3. Checks we're adhering to our own JSDoc comments

GitHub Actions

Checks are optional and can be run manually

GitHub Actions UI for check type declarations

For example, missing or invalid parameters will be spotted by the compiler:

  src/govuk/components/character-count/character-count.mjs:59:35
    59 function CharacterCount ($module, config) {
                                         ~~~~~~
    An argument for 'config' was not provided.

Related changes

This PR completes work we started for v4.4.0:

To assist with code reviews I've split these changes into:

  1. Ensure JSDoc blocks are complete #3102
  2. Configure ESLint to require JSDoc @params etc #3103
  3. Various JSDoc + type checking fixes #2987
  4. Enable type declaration guards #3123
  5. Add (optional) type declaration checks in app, src #3104

Why TypeScript?

We're not actually writing any TypeScript code but this PR includes it to:

  1. Check type information via ESLint using standard standard-with-typescript
  2. Run the TypeScript compiler tsc to check if our code would build without errors

Similar to source maps, we may want tsc to output type declarations in future:

@colinrotherham colinrotherham requested a review from a team as a code owner December 15, 2022 08:42
@colinrotherham colinrotherham linked an issue Dec 15, 2022 that may be closed by this pull request
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 December 15, 2022 11:21 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 December 15, 2022 17:34 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 December 15, 2022 18:34 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 December 15, 2022 18:36 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 December 15, 2022 18:41 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 December 15, 2022 19:02 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 December 15, 2022 19:16 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 December 16, 2022 15:37 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 February 17, 2023 16:53 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 February 17, 2023 17:06 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 February 21, 2023 15:42 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 February 21, 2023 17:09 Inactive
@colinrotherham colinrotherham changed the title Add optional type declaration checks Add (optional) type declaration checks Feb 21, 2023
@colinrotherham
Copy link
Contributor Author

@romaricpascal I've moved the "mandatory" bits to:

This will let TypeScript infer the element types returned by `querySelector` and `querySelectorAll`
from the CSS selector. More info: https://github.com/g-plane/typed-query-selector\#readme
Our vendor directory includes code copied from:

1. https://polyfill.io web service
2. https://www.npmjs.com/package/polyfill-library
3. https://developer.mozilla.org

Feature detection doesn’t necessarily follow completed browser API specifications so we’ll ignore it all as “known working”
The webpack plugin `clean-webpack-plugin` uses incompatible types for its dependant glob and minimatch versions

Removing it as we already set `{ clean: true }` in the webpack config
…of type 'Element'”

We’d missed an `instanceof HTMLElement` check for config.scope
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3104 February 22, 2023 12:56 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.

All ready to go! Thanks so much for your patience with this one for all the reviewing, amends and rebasing to keep it up to date 😄

@colinrotherham colinrotherham merged commit 3c5c6b2 into main Feb 22, 2023
@colinrotherham colinrotherham deleted the lint-types branch February 22, 2023 15:18
@colinrotherham colinrotherham changed the title Add (optional) type declaration checks Add (optional) type declaration checks in app, src Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consistency dependencies Pull requests that update a dependency file interoperability javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotate our code with types and lint to ensure their proper usage
4 participants