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

feat: break down AstroDataError in multiple objects #7919

Closed
wants to merge 1 commit into from

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Aug 2, 2023

Changes

This PR removes the AstroDataError object and instead defines each error as an exported object.

Few things I had to change to adapt some logic:

  • since we no longer have a top-level object, we cannot extract the name of the error at runtime. So I created a new "name" property, which is essentially the name of the error itself;
  • name is now mandatory, so some create*error factory functions add that "name". It's a bit redundant;
  • the code that catches the error at runtime and shows the Astro overlay still needs to understand the name of the error, so I used a trick like import * as AstroErrorData. I tested locally with a project, and it still works;
  • the CI job that checks documentation of the error now fails because the exported AST is different. I have PR ready, but I need help fixing this. Do you think we should turn off the action for now?

Testing

  • I updated some test case that was failing. The rest of the tests should pass.
  • I manually used a project and intentionally created some code that fired an Astro error and checked that the overlays still showed all the relevant information.
  • I manually built an edge middleware test case and checked that the bundle contained only the errors imported by the middleware. It only contains four objects, three shaking works :)

Docs

I didn't add a changelog because this is an internal refactor. Should I create a changelog?

@changeset-bot
Copy link

changeset-bot bot commented Aug 2, 2023

⚠️ No Changeset found

Latest commit: 46974b6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 2, 2023
@ematipico ematipico force-pushed the feat/refactor-errors branch 3 times, most recently from 0f2315f to 48e84a6 Compare August 2, 2023 14:05
@ematipico ematipico marked this pull request as ready for review August 2, 2023 15:13
@ematipico ematipico requested a review from a team as a code owner August 2, 2023 15:13
Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Code looks good, unfortunate that this is needed, but I understand why ESBuild cannot tree shaking unused properties of objects safely.

Don't forget to update the README.md in the src/core/errors folder with the new name property!

Comment on lines 373 to 376
throw new AstroError({
name: 'ContentImportsPluginError',
message: 'Unexpected error processing content collection data.',
});
Copy link
Member

Choose a reason for hiding this comment

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

I'll note that this should be a defined error in error-data with perhaps common causes and stuff. But something to see in another PR perhaps.

The only time where throwing an AstroError that is not defined is ok is if the error is temporary (ex: experimental feature has not been enabled).

@ematipico
Copy link
Member Author

Closing in favour of #7949

@ematipico ematipico closed this Aug 4, 2023
@bluwy bluwy deleted the feat/refactor-errors branch October 8, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants