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

remove unneccesary astro glob #385

Closed
wants to merge 7 commits into from
Closed

Conversation

FredKSchott
Copy link
Member

@FredKSchott FredKSchott commented Apr 26, 2022

Follow up to #365, I don't know if this actually helped our perf in dev, but it can't hurt :) Passing props from getStaticPaths is the official way to pass data down to the page so that you don't need to re-request it a second time.

There are two more perf improvements that I'd like to look into, separate from this, but that we appear to be blocked on:

  1. Removing [slug] and [...slug]: Tony is planning to look into moving docs.astro.build -> astro.build/docs next week, which we should combine with a big "finalize our slugs and remove all of our old redirects".
  2. Vite seems to be building all of our markdown files 3 times, during dev. No idea why this is, but it will need investigation!

cc @delucis

@netlify
Copy link

netlify bot commented Apr 26, 2022

Deploy Preview for astro-docs-2 failed.

Name Link
🔨 Latest commit bdf5743
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/62d7ffc4d1e587000a6bcf31

@FredKSchott
Copy link
Member Author

FredKSchott commented Apr 26, 2022

Hmm, this is a weird bug that I'm only seeing in build (not dev):

 error   Unable to render SidebarToggle!
  
  This component likely uses @astrojs/react or @astrojs/preact,
  but Astro encountered an error during server-side rendering.

@delucis possibly related to this? withastro/astro#2480

@delucis
Copy link
Member

delucis commented Apr 26, 2022

I actually tried this approach initially! But there appears to be a bug with framework components when you pass the content this way. (Errors on build but appears to work in dev, IIRC.)

My wild guess was: Astro uses imports to know to load various assets. When the "import" (via Astro.glob) is inside getStaticPaths some of these don't get picked up. I noticed it with this error but also certain (component-specific, I think) styles.

Obviously would be good to fix this, but FWIW I don't think I actually saw a huge perf difference between the two approaches. Maybe the glob is cached or something. The bigger issue was adb545d

@delucis
Copy link
Member

delucis commented Apr 26, 2022

@delucis possibly related to this? withastro/astro#2480

Hmm, the errors I was getting on that issue were quite different although I don't want to rule it out that they share something.

@FredKSchott
Copy link
Member Author

The bigger issue was adb545d

Agreed, and to make matters worse I think that's being compounded by the same "why is Vite building every markdown file 3 times" issue 🤦

@sarah11918 sarah11918 added the site improvement Some thing that improves the website functionality - ask @delucis for help! label Apr 30, 2022
@sarah11918 sarah11918 added the tsc guidance required from Technical Steering Committee label May 12, 2022
@delucis delucis marked this pull request as draft June 3, 2022 11:07
@delucis
Copy link
Member

delucis commented Jun 3, 2022

As this is still failing, I’ve marked it as a draft to help us triage the PRs tab. I’d love for this to work, so keeping this around for the glorious future in which the bug is fixed.

@delucis delucis added the Blocked Can't deal with because of a bug in Astro, or other blocking reason label Jul 20, 2022
@FredKSchott FredKSchott closed this Aug 8, 2022
@sarah11918 sarah11918 deleted the remove-unneccesary-astro-glob branch December 13, 2022 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Can't deal with because of a bug in Astro, or other blocking reason site improvement Some thing that improves the website functionality - ask @delucis for help! tsc guidance required from Technical Steering Committee
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants