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

Reduce work done by astro sync #9716

Closed
wants to merge 3 commits into from
Closed

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Jan 17, 2024

Changes

image

Testing

Does not affect behavior. Existing tests should pass.

Docs

Does not affect usage.

Copy link

changeset-bot bot commented Jan 17, 2024

🦋 Changeset detected

Latest commit: 2b24054

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Jan 17, 2024
sarah11918
sarah11918 previously approved these changes Jan 17, 2024
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Assuming that this now also says which file doesn't satisfy the content collections type, I'd say that's an improvement! The only way it could be better is if it identified the specific way in which the file did not conform.

matthewp
matthewp previously approved these changes Jan 17, 2024
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

lgtm

@florian-lefebvre
Copy link
Member

I guess we need a changeset?

@lilnasy
Copy link
Contributor Author

lilnasy commented Jan 17, 2024

Assuming that this now also says which file doesn't satisfy the content collections type, I'd say that's an improvement! The only way it could be better is if it identified the specific way in which the file did not conform.

Apparently it already does, but only during build.
image

@Princesseuh
Copy link
Member

Assuming that this now also says which file doesn't satisfy the content collections type, I'd say that's an improvement! The only way it could be better is if it identified the specific way in which the file did not conform.

Apparently it already does, but only during build. image

Those are essentially two different errors, one is because it couldn't generate the types at all (entry malformed), whereas the other one is that a type didn't match

@lilnasy
Copy link
Contributor Author

lilnasy commented Jan 17, 2024

Those are essentially two different errors, one is because it couldn't generate the types at all (entry malformed), whereas the other one is that a type didn't match.

I would've expected it to validate if it parses the content entries. Maybe the odd part is the fact that astro sync reads the markdown files in the first place.

@natemoo-re
Copy link
Member

natemoo-re commented Jan 17, 2024

I would've expected it to validate if it parses the content entries. Maybe the odd part is the fact that astro sync reads the markdown files in the first place.

Honestly that is surprising. I don't think anything about sync needs to perform eager validation, but that could be a follow-up task to fix.

natemoo-re
natemoo-re previously approved these changes Jan 17, 2024
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

#NWTWWHB

@github-actions github-actions bot removed the pr: docs A PR that includes documentation for review label Jan 18, 2024
@lilnasy
Copy link
Contributor Author

lilnasy commented Jan 18, 2024

Sorry for stealing all the reviews, but I think we can not even error in the first place.

@lilnasy lilnasy changed the title qol(content errors): include file whose types could not be generated Reduce work done by astro sync Jan 18, 2024
@lilnasy
Copy link
Contributor Author

lilnasy commented Jan 18, 2024

This is annoying, build depends on sync via shared global state.

image

@lilnasy lilnasy marked this pull request as draft January 18, 2024 02:29
@lilnasy lilnasy dismissed stale reviews from natemoo-re, florian-lefebvre, and matthewp January 18, 2024 02:34

pr was redone

@Princesseuh
Copy link
Member

All of the astro x commands run astro sync at their start, so I assume this is true for many commands

@lilnasy
Copy link
Contributor Author

lilnasy commented Jan 18, 2024

That makes sense. I should clarify what I found problematic working on this.

build depends not just on the functionality of sync, but also on it being performed in the same way npm exec astro sync would have. It involves starting its own vite server,
doing it in the same process, and mutating imported global state so build can consume the result.

The result is errors appearing from unexpected places, and tracking them down being a guessing game.

@lilnasy
Copy link
Contributor Author

lilnasy commented Jan 25, 2024

I did not feel good about the enhacing GenerateContentTypesError because it is a catch-all error. It does not make sense for it to report a specific file name.

The solution I want is where a new AstroError wraps the YamlException close to where it is thrown. That can be in a new PR.

@lilnasy lilnasy closed this Jan 25, 2024
@lilnasy lilnasy deleted the fix/9711 branch January 25, 2024 16: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.

Malformed entry filename is not reported
6 participants