fix(metadata): reset lastIndex of unix manual page regex before testing - #947
fix(metadata): reset lastIndex of unix manual page regex before testing#947btea wants to merge 1 commit into
lastIndex of unix manual page regex before testing#947Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@btea is attempting to deploy a commit to the OpenJS Foundation Team on Vercel. A member of the Team first needs to authorize it. |
PR SummaryLow Risk Overview
Reviewed by Cursor Bugbot for commit 96d492e. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #947 +/- ##
=======================================
Coverage 86.21% 86.21%
=======================================
Files 195 195
Lines 17690 17692 +2
Branches 1609 1609
=======================================
+ Hits 15251 15253 +2
Misses 2433 2433
Partials 6 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| isTextWithUnixManual: ({ type, value }) => { | ||
| QUERIES.unixManualPage.lastIndex = 0; // Reset the lastIndex to ensure proper matching | ||
| return type === 'text' && QUERIES.unixManualPage.test(value); | ||
| }, |
There was a problem hiding this comment.
Can you give an example of something it would miss?
There was a problem hiding this comment.
I dug into this, and honestly: with the current call chain, nothing is observably missed today. isTextWithUnixManual is only used as a visit() predicate (src/generators/metadata/utils/parse.mjs:142), and every successful .test() is immediately followed by the visitor's node.value.replace(QUERIES.unixManualPage, ...) (src/generators/metadata/utils/visitors.mjs:33, same regex instance)
RegExp.prototype[@@replace], step 9, on a global regex resets lastIndex to 0. A failing .test() also resets it, so lastIndex is always 0 at the start of each test. Verified empirically: pre-fix vs post-fix predicates over all of node/doc/api/*.md produce identical matches.
So this is hardening against shared mutable regex state, not a fix for a currently visible bug. Happy to add a small unit test calling the predicate twice to lock it in, or close if you'd rather not carry the extra line.
@nodejs/web-admins this shouldn't happen, we only want builds on |
The ignore step was already set to production only, I've now disabled the "Pull Request Comments" Git functionality, will see if that helps. |
QUERIES.unixManualPageis a global regex (/g), so itslastIndexpersists between calls.UNIST.isTextWithUnixManualuses.test()on this shared instance while visiting text nodes: after a successful match, the next.test()starts from the end of the previous match, intermittently returningfalsefor text that does contain a Unix manual page reference.This resets
lastIndexto 0 before each test so every node is matched from the start.https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test#:~:text=They%20store%20a%20lastIndex%20from%20the%20previous%20match.%20Using%20this%20internally%2C%20test()%20can%20be%20used%20to%20iterate%20over%20multiple%20matches%20in%20a%20string%20of%20text%20(with%20capture%20groups).