feat: hide node and test runner internals from the stack #5714
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
galargh
changed the title
| if (isNodeLocation(reference.location)) { | ||
| // Check if the location is strictly a node location if the node stack | ||
| // is empty. Otherwise, it is OK to accept <anonymous> and index locations | ||
| if (nodeLines.length > 0 || isNodeLocation(reference.location, true)) { |
There was a problem hiding this comment.
Why is there an || here? I don't understand the first part.
There was a problem hiding this comment.
The idea was to only accept a "strictly" node location as the beginning of a node stack but it is no longer relevant after the refactor in bfe4015 (#5714)
| @@ -312,3 +336,17 @@ export function formatLocation( | |||
| return locationPath; | |||
| } | |||
| } | |||
|
|
|||
| function isNodeLocation(location: string, strict: boolean = false): boolean { | |||
There was a problem hiding this comment.
We should probably documment what strict means, or we'll forget it.
There was a problem hiding this comment.
I removed the notion of a strict check in the newest iteration. Instead, I added a new function hasNodeContext which checks whether the context of a reference is for one of the node types. Currently, for the purpose of the function, Array, Promise and SafePromise are considered node types. We can explicitly expand this definition as we find new cases we want to support. I think this approach is safer and easier to understand.
| @@ -108,17 +109,43 @@ function formatSingleError( | |||
| ): string { | |||
| const messageLines = []; | |||
| const stackLines = []; | |||
| const nodeLines = []; | |||
|
|
|||
| for (const line of error.stack?.split("\n") ?? []) { | |||
There was a problem hiding this comment.
It took me a while to understand why i found this harder to follow than what I was expecting, and I think I just got it.
If you were iterrating in reverse order, the algorithm is much simpler/closer to how we dicussed:
let foundNodeFrame = false;
let stackLines = [];
const lines = error.stack?.split("\n") ?? [];
for (let i = lines.length - 1; i >= 0; i--) {
const line = lines[i];
//...
if (!isNodeLocation(reference.location) && !foundNodeFrame) {
// We remove all the frames below the first non-node ones
stackLines = [];
foundNodeFrame = true;
}
stackLines.push(formattedLine);
}
stackLines.reverse();Note that this is pseudocode though, i haven't run it, and it doesn't handle the messageLines
There was a problem hiding this comment.
That's a fair point. It got a tad bit complicated with trying to extract lines from which we build up a message and discarding node lines, but not all of them, ...
This got me thinking and I put together another approach. I'm curious to see what you think about it.
First, I added a new interface StackLine which can hold both the original line that we attempt to parse and a StackReference if we succeed to parse the line into it. With this we can run all the lines through parseStackLine straight away and not worry wether we got undefined back.
Now, among all the parsed lines, we can find the first one which has a defined stack reference. With that in hand, we can build a message from all the lines from the beginning up to that line.
We can also find the last line which does NOT have a reference to node internals. With that, we can easily build the stack from all the lines between the first line with any stack reference and that line. With a special case being that the entire stack is node internals only.
In pseudo-code and without handling special cases the idea is as follows:
const parsedLines = error.stack.map(parseLine);
const firstLineWithReference = parsedLines.findIndex(line => hasReference(line));
const lastLineWithoutNodeReference = parseLines.findLastIndex(line => !hasNodeReference(line));
const message = parsedLines.slice(0, firstLineWithReference).map(formatLine).join("\n");
const stack = parsedLines.slice(firstLineWithReference, lastLineWithoutNodeReference + 1).map(formatLine).join("\n");Let me know what you think :)
There was a problem hiding this comment.
I like it. Much easier to understand.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This PR builds up on custom stack trace parsing introduced in #5700 and implements stack trace filtering. It adds a removal of the node stack references.
Only the node stack references grouped at the end of the stack are removed. They are not removed at all if there are no other stack lines to display to the user.
The stack traces should be much cleaner and easier to comprehend after this change.