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

Notes about String.replace #54387

Open
m-gallesio opened this issue May 25, 2023 · 3 comments
Open

Notes about String.replace #54387

m-gallesio opened this issue May 25, 2023 · 3 comments
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this
Milestone

Comments

@m-gallesio
Copy link

(Moved from rom microsoft/TypeScript-DOM-lib-generator#1562)

I am using TypeScript 5.0.4.
I have noticed the typing of String.replace seem oddly convoluted.

From lib.es5.d.ts:

interface String {
    replace(searchValue: string | RegExp, replaceValue: string): string;
    replace(searchValue: string | RegExp, replacer: (substring: string, ...args: any[]) => string): string;
}

From lib.es2015.symbol.wellknown.d.ts:

interface String {
    replace(searchValue: { [Symbol.replace](string: string, replaceValue: string): string; }, replaceValue: string): string;
    replace(searchValue: { [Symbol.replace](string: string, replacer: (substring: string, ...args: any[]) => string): string; }, replacer: (substring: string, ...args: any[]) => string): string;
}

Like the first parameter is declared as an union, I would expect the second to be the same, at least in the lib.es5 version.

Notably, this makes the following code not compile:

// The second argument is an union of replaceValue and replacer from lib.es5
const replacementSteps: ([RegExp, string | ((substring: string, ...args: any[]) => string)])[] = [
    [/(a|b)/gi, "a"],
    [/(c|d)/gi, match => match.toUpperCase()],
];

let myString = "abcdef";

for (const [searchValue, replacement] of replacementSteps)
    myString = myString.replace(searchValue, replacement); // TS2345 - the second parameter is considered invalid

By adding this into my declaration, the code works:

declare interface String {
    replace(searchValue: string | RegExp, replaceValue: string | ((substring: string, ...args: any[]) => string)): string;
}

I cannot propose a solution other than my last hack, and of course the variants with Symbol would have to be taken into account. Please consider this just a note.

See also this: #54223

@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript Help Wanted You can do this Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript labels May 26, 2023
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone May 26, 2023
@RyanCavanaugh
Copy link
Member

I believe the use of overloads here instead of a union was a holdover from when union types couldn't provide contextual types, or maybe even when we didn't have unions at all. I think this can be updated without breaking anything.

@graphemecluster
Copy link
Contributor

I am happy to modify #50452 to fix this.

@jwasnoggin
Copy link

This definition allows the use of a union while preserving the Symbol.replace bit:

replace<T extends string | ((substring: string, ...args: any[]) => string)>(searchValue: { [Symbol.replace](string: string, replaceValue: T): string; }, replaceValue: T): string;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this
Projects
None yet
Development

No branches or pull requests

4 participants