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

Use magicast for astro add #11772

Merged
merged 8 commits into from
Aug 28, 2024
Merged

Use magicast for astro add #11772

merged 8 commits into from
Aug 28, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Aug 19, 2024

Changes

fix #11836

Use magicast to simplify our current code and reduce the dependency (particularly @babel/generator and @babel/traverse). The generated code is slightly difference where magicast uses recast that injects new lines and trailing commas in some places. I think it's negligable though

Before:

image image

After:

image image

Testing

Ran astro add manually with different setups.

Docs

n/a. internal change.

Copy link

changeset-bot bot commented Aug 19, 2024

🦋 Changeset detected

Latest commit: 5ce4c58

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 the pkg: astro Related to the core `astro` package (scope) label Aug 19, 2024
@bluwy bluwy marked this pull request as ready for review August 27, 2024 09:44
@matthewp
Copy link
Contributor

Should we do this against next instead? astro add is a feature we almost never have to do maintenance on. Glad to reduce the footprint, but not sure doing so in a minor is a good call. If we do it in 5.0 we'll have months to fix bugs this might cause.

@bluwy
Copy link
Member Author

bluwy commented Aug 28, 2024

I did another pass and fix some edge cases where they're adding an integration that is already setup. I think it should be fine in the next minor, or even a patch though since the problem magicast solves is well-scoped. I did some tests locally and it's working fine for a lot of happy paths and edge cases. One thing I realize is that before astro add is executed, we also load and validate the astro config, so we know that at least we're dealing with a well-formed config.

I think bugs could happen for sure, but I don't think it would be too hard to solve. The worst case is that users have to manually update the config themselves.

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.

talked it over and @bluwy feels confident

@bluwy
Copy link
Member Author

bluwy commented Aug 28, 2024

If anything happens, feel free to revert it 😅 (But I'll try to look into it too if it's not urgent)

@bluwy bluwy merged commit 6272e6c into main Aug 28, 2024
14 checks passed
@bluwy bluwy deleted the use-magicast branch August 28, 2024 14:52
@astrobot-houston astrobot-houston mentioned this pull request Aug 29, 2024
@bluwy bluwy mentioned this pull request Aug 31, 2024
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.

Using astro add [...] with empty defineConfig() not automatically adding integration to config
2 participants