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.

Sign up for GitHub

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

Jump to bottom

Switch build.inlineStylesheets default to auto #8118

Merged
merged 8 commits into from
Aug 18, 2023
Merged

Switch build.inlineStylesheets default to auto #8118

merged 8 commits into from
Aug 18, 2023

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Aug 17, 2023 •
edited
Loading

Changes

  • Small stylesheets will now be inlined as <style>tags by default.

Testing

  • Explicitly use inlineStylesheets: never for tests that were written when it was the default
  • for packages/astro/test
  • for packages/integrations/*/test

Docs

The Bundle control section will need to be updated.
PR: withastro/docs#4253 (live, updated)

@changeset-bot
Copy link

changeset-bot bot commented Aug 17, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: b37af2c

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 17, 2023
@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Aug 17, 2023
@lilnasy lilnasy marked this pull request as ready for review August 17, 2023 19:08
@lilnasy
Copy link
Contributor Author

lilnasy commented Aug 17, 2023

Ready for review

@sarah11918
Copy link
Member

Note from docs: there will also need to be a corresponding update to the configuration-reference page via astro.ts!

@lilnasy lilnasy requested a review from a team as a code owner August 17, 2023 19:25
@lilnasy
Copy link
Contributor Author

lilnasy commented Aug 17, 2023

Forgot about it, thanks a lot for the reminder!

matthewp
matthewp approved these changes Aug 17, 2023
View reviewed changes
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.

thanks!

sarah11918
sarah11918 reviewed Aug 17, 2023
View reviewed changes
packages/astro/src/@types/astro.ts
@@ -847,7 +847,7 @@ export interface AstroUserConfig {
* ```js
* {
* build: {
* inlineStylesheets: `auto`,
* inlineStylesheets: `never`,
Copy link
Member

Choose a reason for hiding this comment

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

Heads up that I also updated here, since we typically show a setting that would need to be set, and you wouldn't need to set auto specifically now that it's the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saving me again!
🙏

@sarah11918
Copy link
Member

  1. I'm assuming we should have a changeset since we're changing a default and this could be considered a breaking change for some projects? A notice of what to configure in order to get back to the same behaviour you had if you were relying on the default behaviour?

  2. Asking about protocol here: This PR states there need to be updates to the docs repo. Should I be approving this PR BEFORE those changes are made, or do we think this PR should sit here (and think about what it did!) until there's a corresponding docs PR, so that we know it will happen? 😄

@lilnasy
Copy link
Contributor Author

lilnasy commented Aug 17, 2023

I'm not sure about either of these because I'm not familiar with the implications of merging into next.

Would the changeset become part of 3.0 release notes? Is there a next branch for docs I should open a PR for?

@matthewp
Copy link
Contributor

I would probably not approve this until the docs are updated. I can grab those tomorrow.

@lilnasy lilnasy mentioned this pull request Aug 17, 2023
Merged
sarah11918
sarah11918 reviewed Aug 17, 2023
View reviewed changes
.changeset/smart-numbers-shout.md
'astro': patch
---

Astro is smarter about CSS! Small stylesheets are now inlined by default, and no longer incur the cost of additional requests to your server. Your visitors will have to wait less before they see your pages, especially those in remote locations or in a subway.
Copy link
Member

Choose a reason for hiding this comment

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

lol How can I not approve this?

sarah11918
sarah11918 approved these changes Aug 17, 2023
View reviewed changes
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.

Docs is happy! Have also reviewed the corresponding docs PR, so we're all in good shape! 🙌

@ematipico ematipico merged commit 8a5b0c1 into withastro:next Aug 18, 2023
@astrobot-houston astrobot-houston mentioned this pull request Aug 18, 2023
Merged
@lilnasy lilnasy mentioned this pull request Aug 18, 2023
Closed
@lilnasy lilnasy deleted the inlinestylesheets-auto branch August 22, 2023 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarah11918 sarah11918 sarah11918 approved these changes

@matthewp matthewp matthewp approved these changes

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lilnasy @sarah11918 @matthewp @ematipico