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

Enable experimental.directRenderScript option #7529

Merged
merged 10 commits into from
Apr 10, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Mar 21, 2024

Description (required)

Enable experimental.directRenderScript option so that scripts are rendered at the correct pages. But mainly also, I found this to improve performance during builds. The hoisted script analyzer before took some work to complete:

image

I navigated around the pages locally and everything seem to work right. One report I got (from Erika) is that there may be whitespace issues (as the script is now rendered where it's declared), but I also don't see it happening locally.


I also updated astro and rollup to latest. The current rollup build time locally for me went from 3m25s to 1m55s.

Related issues & labels (optional)

  • Closes #
  • Suggested label:

@bluwy bluwy added the site improvement Some thing that improves the website functionality - ask @delucis for help! label Mar 21, 2024
Copy link

vercel bot commented Mar 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Apr 10, 2024 5:24pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
docs-i18n ⬜️ Ignored (Inspect) Apr 10, 2024 5:24pm

@sarah11918
Copy link
Member

Just a heads up that @delucis is away until Friday, and he should really be the one to take a look at this!

@delucis
Copy link
Member

delucis commented Apr 1, 2024

@sarah11918 This might be a good one to tackle with a mass of Team Docs eyes! The change should be good, but the main thing will be to go through the site carefully checking that nothing has changed visually, so could be a good T&D candidate?

If all looks good, we might consider setting this as the default in Starlight too.

@sarah11918
Copy link
Member

Excellent! We'll review this one Thursday on Talking and Doc'ing and assuming no issues, we'll merge it then! Or, we'll provide feedback as to what might seem off.

@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Apr 3, 2024

Very interested if this works.. I enabled that for the Astrolicious Starlight site which uses Tailwind and it was broken.. However that might have been caused by the combination of Starlight + Tailwind 🤔

https://discord.com/channels/830184174198718474/1070481941863878697/1219595125336969226
https://discord.com/channels/830184174198718474/1070481941863878697/1219734451609665748

@bluwy
Copy link
Member Author

bluwy commented Apr 4, 2024

Is that related to this?

One report I got (from Erika) is that there may be whitespace issues (as the script is now rendered where it's declared), but I also don't see it happening locally.

I don't think we can fix that because that's how the new rendering technique works. You need re-structure/style your elements to ignore the whitespace.

@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Apr 4, 2024

@bluwy It could be due to that, not sure. However since it's Starlight + Tailwind, there is no way I can restructure them in userland. So that would need to be something Starlight or the Starlight Tailwind integration, would need to do 🤔. (And since the question was if this can also be enabled for all Starlight sites, I don't think it can, but docs doesn't use Tailwind, so it should be good for docs I guess).

@sarah11918 sarah11918 added the help - confirm behaviour Walk through the example/issue and confirm this is a general behaviour, or a correct update to make. label Apr 4, 2024
@BryceRussell
Copy link
Member

BryceRussell commented Apr 4, 2024

I am running into an issue with the mobile table of contents, clicking any of the links does not collapse the view. In my console, I am getting these errors:
image

@sarah11918
Copy link
Member

@bluwy One issue reported by Bryce!

@bluwy
Copy link
Member Author

bluwy commented Apr 4, 2024

Thanks @BryceRussell! That seems like a real bug, it looks like the /_astro/TableOfContents... chunk was accidentally deleted because it was inlined in another component. I'll fix that upstream.

@alexanderniebuhr Happy to take a look into it if you can share an example. I think a fix would be to mark certain styles within Starlight to be non-whitespace sensitive so that Tailwind doesn't change it, but I'm not sure where.

@sarah11918 sarah11918 removed the help - confirm behaviour Walk through the example/issue and confirm this is a general behaviour, or a correct update to make. label Apr 4, 2024
@alexanderniebuhr
Copy link
Member

@bluwy forget what I said. It works now, so maybe a recent Astro release fixed it. I tested it two weeks back 🤔 :)

@bluwy
Copy link
Member Author

bluwy commented Apr 9, 2024

This should be ready now. The bug above was fixed in withastro/astro#10686 and I've updated Astro to latest.

Copy link
Member

@BryceRussell BryceRussell left a comment

Choose a reason for hiding this comment

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

LGTM! I tested the preview some more and everything is working with no errors in console

@sarah11918 sarah11918 added the Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)! label Apr 9, 2024
@yanthomasdev yanthomasdev merged commit 6f51db0 into main Apr 10, 2024
6 checks passed
@yanthomasdev yanthomasdev deleted the enable-direct-render-script branch April 10, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)! site improvement Some thing that improves the website functionality - ask @delucis for help!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants