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

🐛 BUG: CSS generated by Astro.glob is included on all pages #3816

Closed
1 task
JLarky opened this issue Jul 4, 2022 · 18 comments
Closed
1 task

🐛 BUG: CSS generated by Astro.glob is included on all pages #3816

JLarky opened this issue Jul 4, 2022 · 18 comments
Assignees
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: styling Related to styles (scope)

Comments

@JLarky
Copy link
Contributor

JLarky commented Jul 4, 2022

What version of astro are you using?

1.0.0-beta.63

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

pnpm

What operating system are you using?

Mac

Describe the Bug

becase Astro.glob('../pages/posts/*.md') will render those markdown files it will also include css files used in them. The issue I noticed is that HRM logic will not unload that css files until you restart you astro dev.

Steps to reproduce:

  1. download astro-hmr-css-bug branch from https://github.com/JLarky/jlarky-blog/tree/astro-hmr-css-bug
  2. run astro dev
  3. open /posts page
  4. now all pages including / and /contacts will have * {color: red} css in them
  5. restart astro dev
  6. now your / or /contacts are fine (i.e. not red) until you click on the /posts again

You can do the same with https://stackblitz.com/github/JLarky/jlarky-blog/tree/astro-hmr-css-bug but instead of restarting astro dev you would do a full page reload of stackblitz app

Possible solutions:

  1. provide Astro.globMetadataOnly that will skip markdown rendering 🤪
  2. I tested that generally Astro will unload css in less crazy setups, so there's probably some kind of leak in this more indirect case. Compare this to https://stackblitz.com/edit/github-8fswvn?file=src%2Fpages%2Fanother.astro that has Astro.glob but if you navigate away from this page css gets properly unloaded

I first saw this issue here: https://youtu.be/EU1MLQMUeXw?t=3490 at 58:58 you can see that I switch to another page but color: red is still applied.

Link to Minimal Reproducible Example

https://stackblitz.com/github/JLarky/jlarky-blog/tree/astro-hmr-css-bug

Participation

  • I am willing to submit a pull request for this issue.
@FredKSchott FredKSchott added - P4: important Violate documented behavior or significantly impacts performance (priority) s1-small feat: styling Related to styles (scope) feat: hmr Related to HMR (scope) labels Jul 6, 2022
@natemoo-re natemoo-re self-assigned this Jul 7, 2022
@natemoo-re natemoo-re changed the title 🐛 BUG: HMR not always unloads css generated by Astro.glob 🐛 BUG: CSS generated by Astro.glob is included on all pages Jul 19, 2022
@natemoo-re natemoo-re added s2-medium and removed s1-small feat: hmr Related to HMR (scope) labels Jul 19, 2022
@natemoo-re
Copy link
Member

So I looked into this during #3932.

Seems like this is not related to HMR, but is the result of how we're crawling the module graph to discover CSS during dev. This is probably a bigger issue to tackle.

@natemoo-re natemoo-re removed their assignment Jul 19, 2022
@JLarky
Copy link
Contributor Author

JLarky commented Jul 20, 2022

So I looked into this during #3932.

Seems like this is not related to HMR, but is the result of how we're crawling the module graph to discover CSS during dev. This is probably a bigger issue to tackle.

thank you for looking into this :) any plans to introduce globs without rendering markdown (just frontmatter) though? :)

@FredKSchott
Copy link
Member

@natemoo-re anything that we can do here before v1.0 next week?

@natemoo-re
Copy link
Member

@FredKSchott and I discussed this with the rest of the team. Given how close we are to v1.0.0, we won't be able to explore this feature in time. However, the ability to glob files while not importing any CSS (as @JLarky suggested) sounds extremely useful and is something we'd like to tackle in the future.

@natemoo-re natemoo-re removed their assignment Aug 5, 2022
@Nike682631
Copy link

@natemoo-re Is this issue available to work on?

@FredKSchott
Copy link
Member

We're beginning to plan our post-v1.0 roadmap!
Please comment here if this issue is affecting you so that we can get a feel for its priority: withastro/roadmap#247

@matthewp matthewp assigned matthewp and unassigned matthewp Nov 4, 2022
@matthewp matthewp added - P2: has workaround Bug, but has workaround (priority) and removed - P4: important Violate documented behavior or significantly impacts performance (priority) labels Nov 4, 2022
@matthewp
Copy link
Contributor

matthewp commented Nov 4, 2022

Relabelled this as a p2, not because it's not important, it's very important. So important that @bholmesdev has been spending weeks working on a solution for this. Relabelled because p4 is for documented behavior bugs and we do not document that Astro.glob usage will not bleed styles.

@bholmesdev bholmesdev self-assigned this Nov 4, 2022
@bholmesdev
Copy link
Contributor

Indeed! @JLarky I suggest checking out the Content Schemas RFC for an early look, particularly the "Render Content" piece of the proposal. Open for feedback on the user-facing API 👍
withastro/roadmap#373

@Enteleform
Copy link
Contributor

@matthewp What is the workaround for this issue?

@bholmesdev
Copy link
Contributor

@Enteleform Sadly no workaround today. This is a fundamental issue with how Vite handles CSS injection by default, and requires Astro core changes to override.

@Enteleform
Copy link
Contributor

@bholmesdev Oh, I thought the p2-has-workaround tag meant there was a workaround for this.

@bholmesdev
Copy link
Contributor

@Enteleform Ah, didn't notice that p2 flag. I agree that's misleading, apologies!

@bholmesdev bholmesdev added - P3: minor bug An edge case that only affects very specific usage (priority) and removed - P2: has workaround Bug, but has workaround (priority) labels Nov 4, 2022
@lloydjatkinson
Copy link
Contributor

I've also encountered this but I think I've discovered some other side effects too.

image

If a page uses glob to load frontmatter of all markdown files (for example, to list the available posts) the following happens:

  • CSS is loaded more than once
  • Fonts are loaded more than once (eg if fonts are setup in css)
  • Bundle sizes become sometimes double or more, as fonts seem to be loaded twice (caching seems to not be happening?)
  • The name of the "first" (seems to be alphabetical) markdown file that was loaded influences the name of the bundle file names. On the homepage /index this appears in the HTML: image
  • A real impact is happening on sites that use glob, consider the following graph. By making no changes at all to /index.astro, markdown files in /pages are able to drastically influence the lighthouse score. New markdown files then alter the performance again. This can impact user perception and content layout shift.
    image

@cocoliliace
Copy link

I was facing the same issue. I resolved it by changing my astro version to 2.0.0-beta.x, where they have the getCollection function implemented. There are some examples on git where we can dig around and get a sense of what it does. @bholmesdev also has a nice proposal + video of the beta API here: withastro/roadmap#373

@pReya
Copy link
Contributor

pReya commented Feb 23, 2023

Content Collections from Astro 2.0 don't fix this on their own. Content collections are not a replacement for Astro.glob(). There are no default routes for content elements from a collection. So if you want to use Astro.glob() to load .mdx content files (e.g. to build menus or dyamic elements) that still need to be accessed via their default route then content collections are not a replacement on their own. You need to create a rout for them manually – only then will content collections fix this problem.

This bug still persists and makes Astro really unusable for larger projects. Flagging this as a "minor bug" is ridiculous. This will break styling and performance on any pages that use Astro.glob().

@bholmesdev
Copy link
Contributor

Thanks for sharing @ChingChang9! Yes, content collections are the intended fix for script and style bleed when using MDX files. We will unfortunately be closing this issue as a "will not fix" for Astro.glob because of this.

If you have a compelling use case where content collections cannot be used, and style bleed is a critical issue, please open a discussion on our roadmap. Thanks everyone!

@pReya
Copy link
Contributor

pReya commented Feb 27, 2023

@bholmesdev Did you want to say that "Content collections + manual routing" are the intended fix? As I said in my last post, Content collections alone do not fix this.

@bholmesdev
Copy link
Contributor

@pReya yes, this is the intended flow when reusing your content in multiple places. i.e. as manual routes, as a collection import for a landing page, as an RSS feed, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: styling Related to styles (scope)
Projects
None yet
Development

No branches or pull requests

10 participants