Skip to content

fix: only pass the groups object to a replacer when the pattern has named groups - #342

Open
theRizwan wants to merge 1 commit into
Rich-Harris:masterfrom
theRizwan:fix-replace-regexp-semantics
Open

fix: only pass the groups object to a replacer when the pattern has named groups#342
theRizwan wants to merge 1 commit into
Rich-Harris:masterfrom
theRizwan:fix-replace-regexp-semantics

Conversation

@theRizwan

Copy link
Copy Markdown
Contributor

_replaceRegexp passes match.groups as the trailing argument to a replacer function on every call:

return replacement(match[0], ...match.slice(1), match.index, str, match.groups)

String.prototype.replace only passes that argument when the pattern actually contains named capture groups. When it does not, match.groups is undefined and magic-string hands the replacer one extra argument that the reference implementation never sends.

'abc'.replace(/b/g, (...args) => (console.log(args), 'Z'))
// [ 'b', 1, 'abc' ]

new MagicString('abc').replace(/b/g, (...args) => (console.log(args), 'Z'))
// [ 'b', 1, 'abc', undefined ]

Replacers written against the documented signature are affected whenever they read from the end of the argument list rather than a fixed position, which is the usual way to reach the offset or the source string from a variadic replacer:

const replacer = (...args) => {
  const source = args.at(-1)     // the source string natively, `undefined` here
  const offset = args.at(-2)     // the offset natively, the source string here
  // ...
}

Replacers with a fixed arity are unaffected, since the extra argument is simply dropped, which is why the existing replace function offset test passes either way.

Fix

Pass the groups object only when the match has one, matching the documented behaviour the source already links to. Named groups keep working exactly as before.

Tests

Two tests, both differential against String.prototype.replace on the same input, one with a named group and one without. The no-named-group test fails on master with the extra undefined and passes with the fix; the named-group test guards against over-correcting.

pnpm run lint, pnpm run typecheck and pnpm test (264 tests) all pass locally.

This is the remaining String.prototype.replace divergence noted in #340, split out as its own change; the two do not overlap in the diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant