Skip to content

Conversation

@cdsiats
Copy link
Contributor

@cdsiats cdsiats commented Aug 18, 2025

What is this PR for?

  • Just updating content and/or documentation
  • This fixes issue #_____
  • Spot fix (no issue #)
  • Merging WIP feature #_____
  • Merging done feature #_____
  • This is a merge from a version branch
  • Adding/Updating tests
  • This is a conflict resolution
  • This is a branch cleanup
  • Other: ________________

I verify that...

  • I have logged my time in the commits
  • I have logged my time in this PR
  • I have tagged all the relevant issues
  • I am using VS Code for type checks and linting, or
  • I have ran npm run test with no errors
  • I have manually checked that this bug or feature is working

@cblanquera
Copy link
Member

Sorry it took a while to review this. While I understand the intent, this change is not a 1:1 drop in replace and would break on a few cases.

The logic for the original code:

  1. Builds the token character by character, checking regexp.test(value + char) at each step.
  2. As soon as adding the next char breaks the match, it returns the previous value (the longest string that still matched).
  3. If nothing ever matched, it returns undefined.

The logic for the proposed optimization:

  1. Takes the substring from index and runs one regex match against it.
  2. Forces the match to start at the beginning of that slice (match.index === 0).
  3. Removes the g flag so lastIndex state can’t corrupt results.
  4. If there’s a match at the beginning, it returns that matched span; otherwise undefined.

If I pass regexp /^\d+$/ and code 12ab, it won’t match at all (because $ forces end-of-slice), whereas the original would correctly return 12. Though incredibly inefficient, the original grows until the longest full match and then stops at the right spot.

I am looking for a better way to express this logic, but accepting this PR as is, is risky.

@cblanquera cblanquera closed this Dec 19, 2025
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.

2 participants