Fix(47969): String.prototypr.replace docs fix#50041
Conversation
Co-authored-by: graphemecluster <graphemecluster@gmail.com>
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
typescript-bot
added
For Milestone Bug
|
@sandersn Shouldn't this be |
Co-authored-by: graphemecluster <graphemecluster@gmail.com>
| * Replaces text in a string, using a regular expression or search string. | ||
| * @param searchValue A string to search for. | ||
| * @param replaceValue A string containing the text to replace for every successful match of searchValue in this string. | ||
| * @param replaceValue A string containing the text to replace. When the searchvalue is a string, only the first match is replaced. If the searchValue is a Regexp, all matches are replaced if the g flag is set. Otherwise only the first one is. |
There was a problem hiding this comment.
There's an "interesting" aspect where "aaa-a".replace(/a/gy, "x") is xxx-a. I don't know how detailed we want it to be, but "all matches are replaced if the g flag is set" is technically not correct—and arguably less correct than before.
There was a problem hiding this comment.
MDN's documentation for the sticky flag says that it ignores the global flag. If that's true, then I think it's fine to document it as-is, since you'll presumably find out about y long after g and hopefully read about the override.
However, is that consistent with your example? If there's some kind of interaction, it might be worth documenting.
There was a problem hiding this comment.
There's an open issue for it: mdn/content#1454 I just haven't gotten around to fix it.
edit: https://github.com/microsoft/TypeScript/blob/main/.github/pr_owners.txt to update this (comes from here) |
Done. Thanks, @orta! |
|
Looks pretty good but I'd like to address @Josh-Cena 's point about the sticky flag before merging. |
| * Replaces first match with string or all matches with RegExp. | ||
| * @param searchValue A string or RegExp search value. | ||
| * @param replaceValue A string containing the text to replace for match. | ||
| * Passes a string and {@linkcode replaceValue} to the [Symbol.replace] method on {@linkcode searchValue}. This method is expected to implement its own replacement algorithm. |
There was a problem hiding this comment.
| * Passes a string and {@linkcode replaceValue} to the [Symbol.replace] method on {@linkcode searchValue}. This method is expected to implement its own replacement algorithm. | |
| * Passes a string and {@linkcode replaceValue} to the `[Symbol.replace]` method on {@linkcode searchValue}. This method is expected to implement its own replacement algorithm. |
(Suggestion, dunno if the stdlib has its own style rules about JSDoc. Also @@replace is technically the name for the method—it's only accessed via Symbol.replace)
There was a problem hiding this comment.
I like the formatting improvement, but I think @@replace is only useful for people who have read the spec.
There was a problem hiding this comment.
Yeah, I'm fine with [Symbol.replace]. FWIW, on MDN we consistently use RegExp.prototype[@@replace]() so I hope it's not that obscure, but I can understand.
| * @param searchValue A string to search for. | ||
| * @param replaceValue A string containing the text to replace for every successful match of searchValue in this string. | ||
| * @param searchValue A string or regular expression to search for. | ||
| * @param replaceValue A string containing the text to replace. When the {@linkcode searchValue} is a string, only the first match is replaced. If the {@linkcode searchValue} is a RegExp, all matches are replaced if the g flag is set. Otherwise only the first one is. |
There was a problem hiding this comment.
I think I would reword this to highlight the interesting global regex case first, and try to cover strings and sticky regexes later.
| * @param replaceValue A string containing the text to replace. When the {@linkcode searchValue} is a string, only the first match is replaced. If the {@linkcode searchValue} is a RegExp, all matches are replaced if the g flag is set. Otherwise only the first one is. | |
| * @param replaceValue A string containing the text to replace. When the {@linkcode searchValue} is a RegExp, all matches are replaced if the g flag is set (taking into account the y flag if present). Otherwise {@linkcode searchValue} only the first match is replaced. |
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
Once @Josh-Cena's latest suggestion is adopted (along with my if -> is fix)
Fixes #47969