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.

Sign up for GitHub

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

Jump to bottom

Various JSDoc + type checking fixes #2987

Merged
merged 15 commits into from
Feb 6, 2023
Merged

Various JSDoc + type checking fixes #2987

merged 15 commits into from
Feb 6, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Nov 9, 2022 •
edited by romaricpascal
Loading

This PR makes a few catch-up tweaks and suggestions for our js.md JavaScript style guide

  1. JSDoc for components
  2. Module check in constructor $module
  3. Module property check !this.$module calling .init()
  4. Adjusted named export guidance after tackling Rollup “synthetic default”

Note: Some code changes were made after setting up JavaScript Standard Style with the "type declaration support" shareable ESLint config that helped spot potential problems. We may want to enable this in future.

Linting JSDoc type declarations

We recently ensured all JSDoc comments compile correctly via npm run build:jsdoc
(See Review app JavaScript Documentation)

Review app JSDoc output

Linting JSDoc type declarations spotted things like:

  1. Optional params not marked with square brackets [param]
  2. Function @param called using previous implementations, not matching JSDoc
  3. Function @return different to actual return type, JSDoc not matching usage

Which continues 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

@colinrotherham colinrotherham added this to the v4.4.0 milestone Nov 9, 2022
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2987 November 9, 2022 08:58 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2987 November 9, 2022 15:38 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2987 November 9, 2022 15:46 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2987 November 9, 2022 16:05 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2987 November 9, 2022 16:16 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2987 November 9, 2022 17:59 Inactive
@colinrotherham colinrotherham changed the title Various JSDoc type fixes Various I18n related JSDoc type fixes Nov 9, 2022
romaricpascal
romaricpascal reviewed Nov 9, 2022
View reviewed changes
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.

Cheers for going through all these type checks 🙌🏻 . I'm only half way through the files, but I'm sharing my current comments in case you want to have a look.

package.json.unit.test.js Outdated Show resolved Hide resolved
src/govuk/components/accordion/accordion.mjs Outdated Show resolved Hide resolved
src/govuk/components/accordion/accordion.mjs Outdated Show resolved Hide resolved
src/govuk/components/character-count/character-count.mjs Outdated Show resolved Hide resolved
src/govuk/components/character-count/character-count.unit.test.mjs Outdated Show resolved Hide resolved
src/govuk/components/character-count/character-count.unit.test.mjs Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2987 November 9, 2022 18:26 Inactive
colinrotherham
colinrotherham commented Nov 9, 2022
View reviewed changes
src/govuk/i18n.mjs Outdated Show resolved Hide resolved
colinrotherham
colinrotherham commented Nov 9, 2022
View reviewed changes
src/govuk/i18n.mjs Outdated Show resolved Hide resolved
@colinrotherham colinrotherham marked this pull request as ready for review November 9, 2022 18:36
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2987 November 9, 2022 21:22 Inactive
@colinrotherham colinrotherham changed the title Various I18n related JSDoc type fixes Various I18n related JSDoc + type checking fixes Nov 10, 2022
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2987 November 10, 2022 08:59 Inactive
36degrees
36degrees approved these changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@36degrees 36degrees left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've been through and taken another look for anything that might be a breaking change and can't see anything either. Let's get this merged, we can always iterate.

Thanks for all your hard work on this, @colinrotherham 🙌🏻

colinrotherham and others added 10 commits February 6, 2023 17:12
@colinrotherham
Fix compiler “This expression is not callable” for outdent
27deff0
@colinrotherham
Fix compiler “Expected 1 arguments, but got 2”
5f3cc7a
@romaricpascal @colinrotherham
Use generics for typing nodeListForEach
503e04f 
This allows to forward the type of the `NodeListOf` it iterates on to the parameters of the callback.
In turn this should allow to drop quite a few `instanceof` checks when using the function by properly typing the `NodeList` we give it.

Interesting links:
- [The `@template` JSDoc tag](https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#template)
- [How to constrain the generic type](https://stackoverflow.com/a/54631901)
@colinrotherham
Fix compiler “Property 'xxxx' does not exist on type 'xxxx'”
d68b694 
Various `instanceof` additions to help the `tsc` compiler discover potential issues

For example a mouse or keyboard `event.target` might not necessarily be the element we expect
@colinrotherham
Fix compiler “Type 'boolean' is not assignable to type 'string'”
d481700
@colinrotherham
Fix compiler “Type 'number' is not assignable to type 'string'”
b694c4b
@colinrotherham
Fix compiler “Type 'string' is not assignable to type 'PluralRuleName'”
247d086 
I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals

Might be worth another look at `Object.keys()` in future with:

* microsoft/TypeScript#45464
@colinrotherham
Fix compiler “Type 'unknown' is not assignable to type 'string'”
149b133
@colinrotherham
Fix ESLint “Operands of '+' operation must either be both strings or …
d085607 
…both numbers”
@colinrotherham
Fix various param/return types in review app
be9aca7 
Including shared file and helper functions
@colinrotherham
Copy link
Contributor Author

Thanks @romaricpascal @36degrees I'll rebase, fix the conflicts then merge 👍

Two of the commits made it into this one but I'll keep them in here since it was approved first:

@colinrotherham
Fix various param/return types in tooling
4d6762b
@colinrotherham
Fix various param/return types in source
c418bbe 
Regarding fallback value types

* Preferring `null` for DOM method fallbacks
* Preferring `undefined` for string fallbacks
@colinrotherham
Allow compiler to cover “else” branched support checks
0663ab3
@colinrotherham
Move Tab bound handlers to constructor
478f642 
Uses 3x handlers per component versus per tab link
@colinrotherham
Update coding standards with JSDoc changes etc
0718c67 
Also includes:

1. Checks for `$module` on instantiation
2. Checks for selectors on init

We prefer `{Element}` type to `{HTMLElement}` etc to maintain compatibility with `querySelector`
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2987 February 6, 2023 17:19 Inactive
@colinrotherham colinrotherham merged commit 448355f into main Feb 6, 2023
@colinrotherham colinrotherham deleted the fix-types branch February 6, 2023 17:25
@colinrotherham colinrotherham mentioned this pull request Feb 7, 2023
Merged
@36degrees 36degrees mentioned this pull request Feb 10, 2023
Closed
1 task
@colinrotherham colinrotherham added assurance 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) and removed tech debt labels Feb 15, 2023
colinrotherham added a commit that referenced this pull request Jun 9, 2023
@colinrotherham
Always prefer return this in constructor early returns
a44fccd 
This is a “just in case” commit where early ES2015 class implementations would return `undefined` in a subclass constructor (extended component) rather than the subclass instance

Might only affect Safari 9

See: #2987 (comment)
colinrotherham added a commit that referenced this pull request Jun 12, 2023
@colinrotherham
Always prefer return this in constructor early returns
f16f23d 
This is a “just in case” commit where early ES2015 class implementations would return `undefined` in a subclass constructor (extended component) rather than the subclass instance

Might only affect Safari 9

See: #2987 (comment)
colinrotherham added a commit that referenced this pull request Jul 19, 2023
@colinrotherham
Always prefer return this in constructor early returns
fff5d22 
This is a “just in case” commit where early ES2015 class implementations would return `undefined` in a subclass constructor (extended component) rather than the subclass instance

Might only affect Safari 9

See: #2987 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owenatgov owenatgov owenatgov left review comments

@romaricpascal romaricpascal romaricpascal left review comments

@36degrees 36degrees 36degrees approved these changes

Assignees
No one assigned
Labels
assurance 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) javascript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Annotate our code with types and lint to ensure their proper usage
6 participants
@colinrotherham @romaricpascal @36degrees @owenatgov @govuk-design-system-ci @kellylee-gds