fix(@angular/build): preserve error stack traces during prerendering#32872
fix(@angular/build): preserve error stack traces during prerendering#32872clydin merged 1 commit intoangular:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@googlebot I signed it! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error reporting in the prerendering process by adding error assertions and reordering error property access to prioritize stack traces. The review feedback suggests using the logical OR operator (||) instead of nullish coalescing (??) when constructing error messages to ensure that empty strings for stack or message properties do not suppress more descriptive error details.
| return { | ||
| errors: [`An error occurred while extracting routes.\n\n${err.message ?? err.stack ?? err}`], | ||
| errors: [ | ||
| `An error occurred while extracting routes.\n\n${err.stack ?? err.message ?? err.code ?? err}`, |
There was a problem hiding this comment.
Consider using the logical OR operator (||) here instead of nullish coalescing (??). While ?? handles null and undefined, it doesn't handle empty strings. Properties like err.stack or err.message can be empty strings, which would cause this expression to resolve to an empty string, hiding error details. Using || would be more robust as it treats empty strings as falsy and falls back to the next property, ensuring a more descriptive error is always displayed.
| `An error occurred while extracting routes.\n\n${err.stack ?? err.message ?? err.code ?? err}`, | |
| `An error occurred while extracting routes.\n\n${err.stack || err.message || err.code || err}` |
| assertIsError(err); | ||
| errors.push( | ||
| `An error occurred while prerendering route '${route}'.\n\n${err.message ?? err.stack ?? err.code ?? err}`, | ||
| `An error occurred while prerendering route '${route}'.\n\n${err.stack ?? err.message ?? err.code ?? err}`, |
There was a problem hiding this comment.
Consider using the logical OR operator (||) here instead of nullish coalescing (??). While ?? handles null and undefined, it doesn't handle empty strings. Properties like err.stack or err.message can be empty strings, which would cause this expression to resolve to an empty string, hiding error details. Using || would be more robust as it treats empty strings as falsy and falls back to the next property, ensuring a more descriptive error is always displayed.
| `An error occurred while prerendering route '${route}'.\n\n${err.stack ?? err.message ?? err.code ?? err}`, | |
| `An error occurred while prerendering route '${route}'.\n\n${err.stack || err.message || err.code || err}` |
| return { | ||
| errors: [ | ||
| `An error occurred while extracting routes.\n\n${err.message ?? err.stack ?? err.code ?? err}`, | ||
| `An error occurred while extracting routes.\n\n${err.stack ?? err.message ?? err.code ?? err}`, |
There was a problem hiding this comment.
Consider using the logical OR operator (||) here instead of nullish coalescing (??). While ?? handles null and undefined, it doesn't handle empty strings. Properties like err.stack or err.message can be empty strings, which would cause this expression to resolve to an empty string, hiding error details. Using || would be more robust as it treats empty strings as falsy and falls back to the next property, ensuring a more descriptive error is always displayed.
| `An error occurred while extracting routes.\n\n${err.stack ?? err.message ?? err.code ?? err}`, | |
| `An error occurred while extracting routes.\n\n${err.stack || err.message || err.code || err}` |
Reorder the nullish coalescing chain from `err.message ?? err.stack` to `err.stack ?? err.message` so that the full stack trace is preserved when available. Since `err.message` is almost always defined on Error objects, the previous order meant `err.stack` was never reached. Also add `assertIsError(err)` and consistent `err.code` inclusion across all three error-handling locations for improved type safety and debugging. Fixes angular#32503
maruthang
force-pushed
the
fix-32503-prerender-error-stacks
branch
from
f4e029d to
6352ad5
Compare
alan-agius4
added
the
target: patch
3 participants
PR Checklist
PR Type
What is the current behavior?
When prerendering fails, the error output shows only the error message but no stack trace, making debugging very difficult. The nullish coalescing chain
err.message ?? err.stack ?? erralways resolves toerr.message(since it's almost always defined on Error objects), soerr.stackis never reached.Issue Number: #32503
What is the new behavior?
err.stack ?? err.message ?? err.code ?? errat all three error-handling locations inprerender.ts, so the full stack trace is preserved when available.assertIsError(err)for type safety, consistent with the existing pattern at line 370.err.codein all three locations for consistency.Does this PR introduce a breaking change?