Skip to content

type: update SpecialString type to use NonNullable #1361

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

Merged
afc163 merged 2 commits into from
Sep 18, 2025
Merged

type: update SpecialString type to use NonNullable #1361

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b82dec0
refactor: update SpecialString type to use NonNullable
afc163 Sep 18, 2025
38d86a9
Merge branch 'master' into type/fix-dataIndex-infer
afc163 Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export interface RenderedCell<RecordType> {
export type Direction = 'ltr' | 'rtl';

// SpecialString will be removed in antd@6
export type SpecialString<T> = T | (string & unknown);
export type SpecialString<T> = T | (string & NonNullable<unknown>);
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

Using NonNullable<unknown> is equivalent to unknown since unknown is already non-nullable. This change has no practical effect. Consider using NonNullable<{}> or {} instead if the intent is to exclude null/undefined from the intersection.

Suggested change
export type SpecialString<T> = T | (string & NonNullable<unknown>);
export type SpecialString<T> = T | (string & {});

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@afc163 afc163 Sep 18, 2025

Choose a reason for hiding this comment

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

@copilot Don't use {} as a type. {} actually means "any non-nullish value".

  • If you want a type meaning "any object", you probably want object instead.
  • If you want a type meaning "any value", you probably want unknown instead.
  • If you want a type meaning "empty object", you probably want Record<string, never> instead.
  • If you really want a type meaning "any non-nullish value", you probably want NonNullable<unknown> instead.eslint@typescript-eslint/ban-types

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Sep 18, 2025

Choose a reason for hiding this comment

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

high

This change correctly applies the string & {} pattern to improve autocompletion. However, the SpecialString< T> type itself appears to have a flaw.

By including T in its definition (T | ...), and since T becomes the RecordType when used in DataIndex, this type incorrectly allows a whole record object to be passed as a dataIndex. This is likely a bug.

A more correct approach would be to remove T from the union. For example:

export type SpecialString = string & NonNullable<unknown>;

This would then require updating its usage in DataIndex on lines 88 and 90 (e.g., ... | SpecialString | ...).

Given this change is out of scope for the current PR, consider creating a follow-up issue or PR to address this typing issue.

Copy link
Copy Markdown
Member Author

@afc163 afc163 Sep 18, 2025

Choose a reason for hiding this comment

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

@gemini-code-assist Don't use {} as a type. {} actually means "any non-nullish value".

If you want a type meaning "any object", you probably want object instead.
If you want a type meaning "any value", you probably want unknown instead.
If you want a type meaning "empty object", you probably want Record<string, never> instead.
If you really want a type meaning "any non-nullish value", you probably want NonNullable<unknown> instead.eslint[@typescript-eslint/ban-types](https://typescript-eslint.io/rules/ban-types)

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Sep 18, 2025

Choose a reason for hiding this comment

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

Thank you for the clarification, @afc163. You are absolutely correct that {} should generally be avoided as a type, and NonNullable<unknown> is indeed the appropriate way to represent 'any non-nullish value' in TypeScript. My apologies for using {} as a shorthand in my previous comment when referring to the pattern.

My primary concern, however, remains with the inclusion of T in the SpecialString<T> definition (T | (string & NonNullable<unknown>)). As T becomes RecordType when SpecialString is used within DataIndex, this type currently allows an entire record object to be passed as a dataIndex, which seems unintended and could lead to runtime issues.

The suggestion to define SpecialString as export type SpecialString = string & NonNullable<unknown>; (without the generic T) and then update its usage in DataIndex was aimed at addressing this specific issue, ensuring that dataIndex only accepts string-like values or DeepNamePath<T>, but not the full RecordType itself.

I believe addressing the T in SpecialString<T> is important for type correctness, even if it's considered out of scope for this particular PR.


export type DataIndex<T = any> =
| DeepNamePath<T>
Expand Down