Skip to content

fix(types): incorrect types for rule options#4633

Merged
escapedcat merged 10 commits intoconventional-changelog:masterfrom
Zamiell:fix-bug
Mar 3, 2026
Merged

fix(types): incorrect types for rule options#4633
escapedcat merged 10 commits intoconventional-changelog:masterfrom
Zamiell:fix-bug

Conversation

@Zamiell
Copy link
Copy Markdown
Contributor

@Zamiell Zamiell commented Mar 2, 2026

Description

The types for several rules are marked as being mutable only, which is a common mistake when writing TypeScript libraries and is probably unintended. This pull request fixes it.

Motivation and Context

This code results in a type error:

const MY_COMMITLINT_RULES = {
  "scope-enum": [ERROR, "always", ["foo", "baz", "baz"]],
} as const;

To work around this, you have to do a pointless type assertion:

const MY_COMMITLINT_RULES = {
  "scope-enum": [ERROR, "always", ["foo", "baz", "baz"] as string[]],
} as const;

This pull request fixes the issue.

How Has This Been Tested?

n/a

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Fix incorrect mutable types for rule option arrays

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Fix incorrect mutable array types in rule configurations
• Change array types to readonly in scope-case and scope-enum rules
• Allow const assertions without unnecessary type casting
Diagram
flowchart LR
  A["Rule Config Types"] -->|"Update scope-case"| B["cases: readonly TargetCaseType[]"]
  A -->|"Update scope-case"| C["delimiters: readonly string[]"]
  A -->|"Update scope-enum"| D["scopes: readonly string[]"]
  A -->|"Update scope-enum"| E["delimiters: readonly string[]"]
  B --> F["Enable const assertions"]
  C --> F
  D --> F
  E --> F
Loading

Grey Divider

File Changes

1. @commitlint/types/src/rules.ts 🐞 Bug fix +8/-2

Make rule option arrays readonly

• Changed cases property in scope-case rule from TargetCaseType[] to readonly TargetCaseType[]
• Changed delimiters property in scope-case rule from string[] to readonly string[]
• Changed scopes property in scope-enum rule from string[] to readonly string[]
• Changed delimiters property in scope-enum rule from string[] to readonly string[]

@commitlint/types/src/rules.ts


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Mar 2, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Readonly fix incomplete 🐞 Bug ✓ Correctness
Description
Only the object-based scope-case/scope-enum option shapes were updated to readonly, but the
array-form config types still require mutable arrays (string[], TargetCaseType[]). This means
as const configs like the PR description example can still fail type-checking, and internal code
still needs type assertions.
Code

@commitlint/types/src/rules.ts[R116-129]

	"scope-case":
		| CaseRuleConfig<V>
-		| ObjectRuleConfig<V, { cases: TargetCaseType[]; delimiters?: string[] }>;
+		| ObjectRuleConfig<
+				V,
+				{ cases: readonly TargetCaseType[]; delimiters?: readonly string[] }
+		  >;
	"scope-delimiter-style": EnumRuleConfig<V>;
	"scope-empty": RuleConfig<V>;
	"scope-enum":
		| EnumRuleConfig<V>
-		| ObjectRuleConfig<V, { scopes: string[]; delimiters?: string[] }>;
+		| ObjectRuleConfig<
+				V,
+				{ scopes: readonly string[]; delimiters?: readonly string[] }
+		  >;
Evidence
RuleConfigTuple is defined as a readonly tuple, which is exactly what as const produces, but
EnumRuleConfig/CaseRuleConfig still model the *value* as mutable arrays. The PR only makes the
object-based scopes/cases/delimiters properties readonly (good), yet the union types still include
EnumRuleConfig/CaseRuleConfig for the array-form, so the original as const friction remains.
This is evidenced by internal configs still requiring explicit type assertions for array-valued
rules.

@commitlint/types/src/rules.ts[54-90]
@commitlint/types/src/rules.ts[110-129]
@commitlint/config-conventional/src/index.ts[14-46]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`RuleConfigTuple` is a readonly tuple, but many rule value types still use mutable arrays (e.g., `string[]`, `TargetCaseType[]`). This makes `as const` configs fail type-checking for the common array-form configs (including the PR description’s example), so the current change is only a partial fix.

## Issue Context
The PR updated only the *object-based* `scope-case`/`scope-enum` option object properties to `readonly ...[]`. However, the array-form configs are still typed as mutable arrays via `EnumRuleConfig` and `CaseRuleConfig`, and internal configs already need type assertions as a workaround.

## Fix Focus Areas
- @commitlint/types/src/rules.ts[54-90]
- @commitlint/types/src/rules.ts[110-129]
- @commitlint/rules/src/scope-enum.ts[5-10]
- @commitlint/rules/src/scope-case.ts[7-13]
- @commitlint/rules/src/header-case.ts[7-11]
- @commitlint/cz-commitlint/src/utils/rules.ts[59-74]
- @commitlint/config-conventional/src/index.ts[14-46]

## Suggested approach
1. Change `EnumRuleConfig` to use `readonly string[]` (and any similar array-valued rule types).
2. Change `CaseRuleConfig` to use `TargetCaseType | readonly TargetCaseType[]`.
3. Optionally align rule implementation generic signatures (e.g., `scopeEnum`, `scopeCase`, `headerCase`, etc.) to accept readonly arrays too.
4. Update internal helpers that narrow/return `string[]` (e.g., `enumRuleIsActive`, `getEnumList`) to work with readonly arrays (or widen appropriately) and remove now-unnecessary type assertions in internal config packages.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Mar 2, 2026

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Mar 2, 2026

@Zamiell can you add some tests?

@Zamiell
Copy link
Copy Markdown
Contributor Author

Zamiell commented Mar 2, 2026

sure: Zamiell@ce22011

(this commit also satisfies the request from our robot overlords above)

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts @commitlint/types rule option types to accept readonly arrays (improving as const compatibility) and adds compile-time regression coverage to prevent the issue from reappearing.

Changes:

  • Update rule option types to use readonly ...[] where appropriate (including object-form configs for scope-case/scope-enum).
  • Add a compile-time type regression test (rules.test-d.ts) covering as const configurations.
  • Enable Vitest typechecking and wire the new *.test-d.ts file into the root typecheck TS config.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
vitest.config.ts Enables Vitest typechecking so compile-time type tests can run in CI.
tsconfig.json Includes *.test-d.ts files and adds a project reference to @commitlint/types to support typechecking across packages.
@commitlint/types/tsconfig.json Excludes *.test-d.ts from the package build output while still allowing root-level typechecking.
@commitlint/types/src/rules.ts Changes several rule option array types to readonly to support as const configs.
@commitlint/types/src/rules.test-d.ts Adds compile-time regression coverage for readonly-friendly rule configs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@escapedcat
Copy link
Copy Markdown
Member

@Zamiell thanks! Please have a look at the feedback from copilot

@Zamiell
Copy link
Copy Markdown
Contributor Author

Zamiell commented Mar 2, 2026

I think Copilot is hallucinating. This is a non breaking change because using readonly like this allows you to either EITHER mutable or non-mutable arrays.

@Zamiell
Copy link
Copy Markdown
Contributor Author

Zamiell commented Mar 2, 2026

Furthermore, even if this was not the case, nobody would mutate their configs AFTER passing it inside of commitlint, that doesn't make any sense.

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Mar 2, 2026

that doesn't make any sense.

You would be surprised about how some devs shoot their own foot. But yeah I think to just address this, we flag this PR as a breaking-change and end of story? @escapedcat will anyway add another breaking change in next release as well, so it's the perfect time for these IMO

@Zamiell
Copy link
Copy Markdown
Contributor Author

Zamiell commented Mar 2, 2026

To be clear, this is strictly not a breaking change though. Just check out my PR and try to use a mutable array to see for yourself.

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Mar 2, 2026

The only further advice I would give here, IMHO, is to reverse the order of the commits, so as to make them be proper unit tests instead of regression tests (in the sense that we should see the red CI in 1st commit but then green CI in the second).

@Zamiell
Copy link
Copy Markdown
Contributor Author

Zamiell commented Mar 2, 2026

This TypeScript playground demonstrates that it is not a breaking change.

The only further advice I would give here, IMHO, is to reverse the order of the commits, so as to make them be proper unit tests instead of regression tests (in the sense that we should see the red CI in 1st commit but then green CI in the second).

Can we squash the commits instead please?

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Mar 2, 2026

Can we squash the commits instead please?

If you do that we can't see the red CI (the proof that there was a problem before the fix).

@Zamiell
Copy link
Copy Markdown
Contributor Author

Zamiell commented Mar 2, 2026

Oh, I see. Let me see if I can reorder the commits.

Zamiell and others added 2 commits March 2, 2026 06:44
…lity

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Zamiell
Copy link
Copy Markdown
Contributor Author

Zamiell commented Mar 2, 2026

Ok, I think it should be good to go now:

$ git log
commit fdcf183034227df343bb417620009aa3b32bdab9 (HEAD -> fix-bug, origin/fix-bug)
Author: Zamiell <5511220+Zamiell@users.noreply.github.com>
Date:   Mon Mar 2 06:44:36 2026 -0500

    fix(types): use readonly arrays for "as const" compatibility

    Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

commit a3ca58ebddb04efd7484d72081aaf503376f438e
Author: Zamiell <5511220+Zamiell@users.noreply.github.com>
Date:   Mon Mar 2 06:44:28 2026 -0500

    test(types): add type-check regression tests for "as const" compatibility

    Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

I force pushed to the branch.

@escapedcat
Copy link
Copy Markdown
Member

Hm, was the expectation that we see a failing test here first?

image

@Zamiell
Copy link
Copy Markdown
Contributor Author

Zamiell commented Mar 2, 2026

Oh, I think it just skipped running CI on that commit because GitHub only runs CI on the most recent commit of a git push (as an optimization) and I pushed them both at the same time. Let me try reverting the last commit and then reverting the revert.

@escapedcat
Copy link
Copy Markdown
Member

Let me try reverting the last commit and then reverting the revert

:D

"I heard you like reverts"

@Zamiell
Copy link
Copy Markdown
Contributor Author

Zamiell commented Mar 2, 2026

ok, we can see now that CI fails: https://github.com/conventional-changelog/commitlint/actions/runs/22575027298/job/65392114597?pr=4633

Do you want me to revert the revert now?

@Zamiell
Copy link
Copy Markdown
Contributor Author

Zamiell commented Mar 2, 2026

ok, revert has been reverted, CI should go green now.

This was referenced Mar 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants