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

docs: better explanation for base configuration #8779

Merged
merged 5 commits into from Oct 17, 2023

Conversation

ematipico
Copy link
Member

Changes

The documentation wasn't extensive enough IMHO. I didn't understand that config.base is also manipulated by Astro accordingly, this is important because base is read by integrations too.

I also added some practical examples.

Testing

N/A

Docs

/cc @withastro/maintainers-docs for feedback!

@ematipico ematipico requested a review from a team as a code owner October 9, 2023 10:12
@changeset-bot
Copy link

changeset-bot bot commented Oct 9, 2023

🦋 Changeset detected

Latest commit: e5f96af

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 Oct 9, 2023
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.

I don't think we need a changeset for this change, but otherwise lgtm! Would also be good to have a pass from docs before merging too.

@delucis
Copy link
Member

delucis commented Oct 9, 2023

When does Astro manipulate config.base? And can we avoid doing so? It seems bad practice for us to mutate user config like that, but if it is required, these docs should probably say WHEN that happens. I’m guessing in astro:config:setup it hasn’t happened yet? Because then it would have happened before another integration has the chance to set base itself. So now we potentially have a pretty painful docs problem of “config.base can be different depending on when you read it”.

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.

For the sake of a quick, helpful PR to docs that is an improvement, I've made some suggestions below that I think can work in the short term.

But after talking with @delucis, I do think we need a more extensive look at this to consider this behaviour fully documented!

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
@ematipico
Copy link
Member Author

ematipico commented Oct 9, 2023

When does Astro manipulate config.base? And can we avoid doing so? It seems bad practice for us to mutate user config like that, but if it is required, these docs should probably say WHEN that happens. I’m guessing in astro:config:setup it hasn’t happened yet? Because then it would have happened before another integration has the chance to set base itself. So now we potentially have a pretty painful docs problem of “config.base can be different depending on when you read it”.

To be honest, I don't know when Astro does the manipulation, but I found this when I was testing a vite plugin, and the plugin received the base that was already manipulated. So I can assume that the manipulation occurs before starting vite. This is a guess.

ematipico and others added 2 commits October 9, 2023 17:15
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
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.

One missing word commented in below, but then Docs is happy with this PR! Would be nice to more fully understand exactly what's happening for documenting more fully later, but I think this is a helpful update as is!

ematipico and others added 2 commits October 17, 2023 04:56
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@ematipico ematipico merged commit 2b8a459 into main Oct 17, 2023
14 checks passed
@ematipico ematipico deleted the docs/better-explaination-for-base branch October 17, 2023 16:06
@astrobot-houston astrobot-houston mentioned this pull request Oct 17, 2023
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.

None yet

4 participants