Add union of replacer function and string to lib d.ts #44348
Conversation
| * @param searchValue A string to search for. | ||
| * @param replacer A string containing the text to replace for every successful match or a function that returns the replacement text. | ||
| */ | ||
| replace(searchValue: string | RegExp, replacer: string | ((substring: string, ...args: any[]) => string)): string; |
There was a problem hiding this comment.
@orta do you know why 3 overloads of replace are needed? It seems like this new one is a supertype of the previous 2, even in es2015.symbol.wellknown, so maybe only the original two can be dropped.
I may be getting the variance backward in es2015.symbol.wellknown, though; could that be why there are 3 overloads?
There was a problem hiding this comment.
Well, if it weren't for those pesky users, one overload in es5.d.ts and one in es2015.symbol.wellknown.d.ts would be plenty. I believe it's about sparing them the cognitive load of parsing through the union and parsing through the doc text and matching up the alternatives in the prose with the alternatives in the type.
Incidentally, these docs are already quite confusing, and the first existing overload in es2015.symbol.wellknown looks to suffer from "copy/paste/forget to update". (I wrote a patch at fbb4939 but when I went to PR I found out I needed an issue number, so I was looking for existing issues and that's how I ended up here.)
But the part I really found confusing was "for every successful match": I guess it's technically true, but what it doesn't say is that only one match is even attempted for strings, or for any RegExps that don't have the g flag.
I suppose I should go report an issue ...
|
I'm going to close this PR. I'm extremely wary of touching commonly used types in the lib, since any change ends up breaking somebody, and I don't think the improvement is worth it. |
Fixes #29789 - replacing #34941 which couldn't be migrated to main ( #44323 )