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

Custom component configs cause TypeScript compiler errors #5606

Open
colinrotherham opened this issue Jan 14, 2025 · 6 comments · May be fixed by #5607
Open

Custom component configs cause TypeScript compiler errors #5606

colinrotherham opened this issue Jan 14, 2025 · 6 comments · May be fixed by #5607
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation) javascript tooling

Comments

@colinrotherham
Copy link
Contributor

colinrotherham commented Jan 14, 2025

Description of the issue

Hello 👋

Nice work with the v5.8.0 release! Mind if I raise a little JSDoc types issue?

In TypeScript projects, the compiler outputs this error for ConfigurableComponent configs:

Type 'ExampleConfig' does not satisfy the constraint 'ObjectNested'.
  Index signature for type 'string' is missing in type 'ExampleConfig'. ts(2344)

Similarly, config properties can't use use arrays or have null values as ObjectNested doesn't support them

Steps to reproduce the issue

Configure tsconfig.json to include GOV.UK Frontend sources or use @types/govuk-frontend (generated via JSDoc)

Actual vs expected behaviour

I've had a look, and the JSDoc is set up to only accept string | boolean | number or other nested objects as allowed properties as part of the ObjectNested definition

Do you think component configs should allow ObjectNested { [key: string]: unknown } instead?

Happy to give a PR a go

@colinrotherham colinrotherham added awaiting triage Needs triaging by team 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) labels Jan 14, 2025
@colinrotherham
Copy link
Contributor Author

Looks like the compiler error would be triggered by the existing components too

Fixed keys like i18n aren't passing [key: string] checks:

Type 'ExampleConfig' does not satisfy the constraint 'ObjectNested'.
  Index signature for type 'string' is missing in type 'ExampleConfig'. ts(2344)

Might be a better idea to use keyof ConfigurationType like this?

- * @template {ObjectNested} [ConfigurationType={}]
+ * @template {Record<keyof ConfigurationType, unknown>} [ConfigurationType=ObjectNested]

@colinrotherham
Copy link
Contributor Author

I've applied the same fixes as #5607 to DefinitelyTyped/DefinitelyTyped@d3b7de6 for @types/govuk-frontend

@romaricpascal
Copy link
Member

Cheers for the issue and the attempt at fixing this 😊

I'm trying to wrap my head around what's going on so I can follow what the fix does, but our type linting seems to work OK with both:

  • the components in GOV.UK Frontend
  • the Design System's CookieBanner (after adding an @augments {ConfigurableComponent<CookieBannerConfig>} which we seem to have forgotten).

Could please you provide a little more details about:

  • if the error tied to a specific version of TypeScript or flag in the configuration which we may be missing (and because of this, missing the error)
  • which part of the code around creating a configurable component trigger the error. I've reduced our CookieBanner to the parts around types and our linting in the Design System site does not complain either, so we may be missing something that triggers the error
/**
 * @augments ConfigurableComponent<ExampleConfig>
 */
class ExampleComponent extends ConfigurableComponent {

  static defaults = {
    anOption: 'aValue'
  }

  static schema = {
    properties: {
      anOption: { type: 'string' }
    }
  }
}

/**
 * @typedef {object} ExampleConfig
 * @property {string} anOption
 */

Cheers 😊

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Jan 27, 2025

Yeah course, give one of these examples a try

Any custom config that doesn't meet extends ObjectNested is affected

Nullable option values

E.g. Using string? or string | null

/**
 * @typedef {object} ExampleConfig
 * @property {string?} anOption - Nullable strings are not allowed
 */

Array option values

/**
 * @typedef {object} ExampleConfig
 * @property {string} anOption - Strings are allowed
 * @property {string[]} brokenOption1 - Arrays of strings not allowed
 * @property {{ text: string }[]} brokenOption2 - Arrays of objects not allowed
 */

Strict literal option values

In TypeScript only, string literal (and string enum) values cause errors

type ExampleConfig = {
  /**
   * String literals are not allowed
   */
  brokenOption: 'red' | 'blue'
}

Options with only known keys

In TypeScript only, all configs that don't extend { [key: string]: string } cause errors

type ExampleConfig = {
+ [key: string]: string
  anOption: string
}

This is because all configs must match extends ObjectNested with the [key: string] index

It prevents unknown or legacy properties like config.unknownProperty being caught by the compiler

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Jan 27, 2025

Did you notice that "strict": true errors only appear for npm run lint:types and not in the editor?

Flipping the tsconfig.json reference order fixes it:

{
  "extends": "../../tsconfig.base.json",
  "files": [],
  "references": [
    {
-     "path": "./tsconfig.dev.json"
+     "path": "./tsconfig.build.json"
    },
    {
-     "path": "./tsconfig.build.json"
+     "path": "./tsconfig.dev.json"
    }
  ]
}

Wonder if this was a recent TypeScript compiler change?

@colinrotherham
Copy link
Contributor Author

I'm trying to wrap my head around what's going on so I can follow what the fix does…

In short, I've swapped ObjectNested for Partial<Record<keyof ConfigurationType, unknown>>

This allows users with type checking enabling to create their own configs as they wish

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) javascript tooling
Projects
Status: Backlog 🏃🏼‍♀️
Development

Successfully merging a pull request may close this issue.

3 participants