Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: show file name with invalid frontmatter errors for MDX #12355

Merged
merged 7 commits into from
Nov 6, 2024

Conversation

apatel369
Copy link
Contributor

@apatel369 apatel369 commented Nov 2, 2024

Changes

  • What does this change?
    -astro build does not report which file has an invalid frontmatter for MDX files.
    This PR fixes that.

After the fix, the error looks like the below:

Screenshot 2024-11-02 at 10 15 14 AM

Closes #12352

Testing

It was manually tested by running a local build.

Docs

Not needed as it just enhances existing error messages.

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Nov 2, 2024
.changeset/wise-bulldogs-jump.md Outdated Show resolved Hide resolved
@@ -232,7 +234,7 @@ async function syncContentCollections(
{
...AstroErrorData.GenerateContentTypesError,
hint,
message: AstroErrorData.GenerateContentTypesError.message(safeError.message),
message: AstroErrorData.GenerateContentTypesError.message(`${safeError.message} in ${fileId}`),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of using the file id in the message, you could pass it under loc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed fileId and added location

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the error now shows the file name on the stack trace now:
image

Though for some reason the error is getting printed twice, and the second error didn't have that information. But anyways the first is fixed here.

Second error image

packages/astro/src/core/sync/index.ts Outdated Show resolved Hide resolved
@apatel369
Copy link
Contributor Author

Looks like the error now shows the file name on the stack trace now: image

Though for some reason the error is getting printed twice, and the second error didn't have that information. But anyways the first is fixed here.

Second error

It seems to be an existing issue with how AstroError uses location. Do I need to address this in this PR, or should I pass the location information differently?

@Princesseuh
Copy link
Member

Pushed a few changes to improve errors with sync in general, it was using a lot of custom code to do things we already have.

  • Error message is now only shown once
  • Location is included in the CLI when an error crashes the process
  • Reworded error message's hint to make it more clear it can be caused by an error in a file
image

Comment on lines +136 to +137
await sync({ flags });
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a global handler for CLI that handles more stuff (telemetry, error formatting etc) so this wasn't really useful.

Comment on lines +301 to +304
if (showFullStacktrace && err.loc) {
output.push(` ${bold('Location:')}`);
output.push(` ${underline(`${err.loc.file}:${err.loc.line ?? 0}:${err.loc.column ?? 0}`)}`);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was something removed by Fred's previous pass on errors making them shorter 😃

I added it back, but only when the full stack trace is shown (which is either in debug in dev, or on full crashes in the CLI), so it should be shown in only contexts where it's not available elsewhere.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love to see less try catches around

@bluwy bluwy merged commit c4726d7 into withastro:main Nov 6, 2024
11 of 13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs pr pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

astro build does not report which file has an invalid frontmatter for MDX files
4 participants