Fix performance issue when throwing errors with stackTraceLimit = 0 and improve @fs URL parsing #8531
Conversation
|
@sheremet-va 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
Co-authored-by: sheremet-va <[email protected]>
Co-authored-by: sheremet-va <[email protected]>
packages/utils/src/source-map.ts
Outdated
| return null | ||
| } | ||
|
|
||
| let file = url |
There was a problem hiding this comment.
@copilot do not copy code form parseSingleV8Stack, this situation is not possible in firefox/webkit
There was a problem hiding this comment.
Fixed in fa0515f - removed the file path processing code that was copied from parseSingleV8Stack. Firefox/Safari stack traces now preserve their original file paths without stripping the file:// prefix or normalizing paths.
test/core/test/utils.spec.ts
Outdated
| }) | ||
|
|
||
| describe('parseErrorStacktrace performance with stackTraceLimit = 0', () => { | ||
| test('should not hang when parsing errors with large messages and no stack trace', () => { |
test/core/test/utils.spec.ts
Outdated
| expect(result).toEqual([]) | ||
| }) | ||
|
|
||
| test('should early return for invalid Firefox/Safari stack trace lines', () => { |
test/core/test/utils.spec.ts
Outdated
|
|
||
| test('should still parse valid Firefox/Safari stack traces correctly', () => { | ||
| // Ensure we didn't break valid stack trace parsing | ||
| const validStackLine = 'functionName@file:///path/to/file.js:123:45' |
There was a problem hiding this comment.
Added in 6394128 - test now covers both file:// and https:// protocols for Firefox/Safari stack trace parsing. The test correctly expects the pathname only (/path/to/file.js) for https URLs as per the extractLocation function behavior.
Co-authored-by: sheremet-va <[email protected]>
Co-authored-by: sheremet-va <[email protected]>
test/core/test/utils.spec.ts
Outdated
| @@ -1,4 +1,5 @@ | |||
| import { assertTypes, deepClone, deepMerge, isNegativeNaN, objDisplay, objectAttr, toArray } from '@vitest/utils' | |||
| import { parseErrorStacktrace, parseSingleFFOrSafariStack } from '@vitest/utils/source-map' | |||
There was a problem hiding this comment.
@copilot 'parseErrorStacktrace' is defined but never used
There was a problem hiding this comment.
Fixed in b1c130b - removed the unused parseErrorStacktrace import from the test file. Only parseSingleFFOrSafariStack is now imported as it's the only function actually used in the tests.
Co-authored-by: sheremet-va <[email protected]>
Co-authored-by: sheremet-va <[email protected]>
| for (let i = 0; i < line.length; i++) { | ||
| if (line[i] === '@') { | ||
| const candidateLocation = line.slice(i + 1) | ||
| if (candidateLocation.includes(':') && candidateLocation.length >= 3) { |
There was a problem hiding this comment.
The 3 represents the minimum length for a valid location string. A valid location must follow the pattern filename:line (e.g., a:1), so it needs at least 3 characters: 1 for the filename, 1 for the colon, and 1 for the line number. This prevents processing invalid short strings that can't represent valid stack trace locations.
There was a problem hiding this comment.
Added explanatory comments in 770c88a for both the validation logic and the minimum length check. The comments explain that 3 represents the minimum valid location format: 1 character for filename + 1 colon + 1 character for line number (e.g., "a:1").
…tions Co-authored-by: sheremet-va <[email protected]>
✅ Deploy Preview for vitest-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for vitest-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
AriPerkkio
left a comment
AriPerkkio
left a comment
There was a problem hiding this comment.
Repro from #6039 seems fixed with these changes. 29.20s before, 91ms now.
import { it } from "vitest";
it("should not take ages...", async () => {
const obj = Object.fromEntries(
[...Array(40).keys()].map((i) => [`prop${i}`, i])
);
class PrettyError extends globalThis.Error {
constructor(e: unknown) {
Error.stackTraceLimit = 0;
super(JSON.stringify(e));
}
}
throw new PrettyError(obj);
});
Added explanatory comments for the minimum length validation in the Firefox/Safari stack trace parsing logic. The value
3represents the minimum valid location format: 1 character for filename + 1 colon + 1 character for line number (e.g., "a:1").💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.