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 compiler error "does not satisfy the constraint 'ObjectNested'" #5607

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

colinrotherham
Copy link
Contributor

This PR fixes #5606

Affects ConfigurableComponent<CustomConfig> in v5.8.0 where CustomConfig doesn't extend ObjectNested

   * @virtual
-  * @template {ObjectNested} [ConfigurationType={}]
+  * @template {Partial<Record<keyof ConfigurationType, unknown>>} [ConfigurationType=ObjectNested]
   * @template {Element & { dataset: DOMStringMap }} [RootElementType=HTMLElement]
   * @augments GOVUKFrontendComponent<RootElementType>

I've shared the config type via Schema Schema<ConfigurationType> for use in @param and @returns too

This may not have affected JSDoc due to The Empty Object Type in TypeScript

@domoscargin domoscargin added awaiting triage Needs triaging by team javascript 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) and removed awaiting triage Needs triaging by team labels Jan 15, 2025
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.

Looks good, thanks for the update 🙌🏻

It makes sense to describe the ConfigurationType received by the functions that receive a configuration from the markup or component constructor with Partial<Record<keyof ConfigurationType, unknown>> as it's more representative of the expected situation.

Only one little stylistic suggestion for that for loop in normaliseDataset which is a bit heavy with the casting in the middle of it, would be great to have that as a variable before the loop. I'll approve in the meantime.

Comment on lines 169 to 177
for (const entries of /** @type {[keyof ConfigurationType, SchemaProperty | undefined][]} */ (
Object.entries(Component.schema.properties)
)) {
const [namespace, property] = entries
const field = namespace.toString()
Copy link
Member

Choose a reason for hiding this comment

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

Recording what we understood during the review

  • namespace is a keyof ConfigurationType which will be passed to extractConfigByNamespace which also requires a keyof ConfigurationType
  • field needs to be a string so it can be used to access dataset (a DOMStringMap and fill out (an ObjectNested, whose keys are string)

Copy link
Member

Choose a reason for hiding this comment

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

suggestion Having the casting outside of the for loop would increase readability:

Suggested change
for (const entries of /** @type {[keyof ConfigurationType, SchemaProperty | undefined][]} */ (
Object.entries(Component.schema.properties)
)) {
const [namespace, property] = entries
const field = namespace.toString()
const entries = /** @type {[keyof ConfigurationType, SchemaProperty | undefined][]} */ (
Object.entries(Component.schema.properties)
)
for (const entry of entries) {
const [namespace, property] = entry
// Cast the `namespace` to string so it can be used to access the dataset
const field = namespace.toString()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @romaricpascal @domoscargin

Just pushed. How's this? ab3d188

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM - running checks now

@domoscargin domoscargin merged commit 42a1c61 into alphagov:main Feb 3, 2025
45 checks passed
@domoscargin
Copy link
Contributor

Thanks @colinrotherham!

@colinrotherham colinrotherham deleted the custom-config-types branch February 3, 2025 16:24
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom component configs cause TypeScript compiler errors
3 participants