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

Fix location of SolidJS pre-hydration code #3140

Merged

Conversation

hippotastic
Copy link
Contributor

Changes

Testing

Tested locally on Windows 10 in dev mode, build, and SSR using Netlify adapter.

Docs

This is not a visible change, just a code fix/refactor.

* Run before hydration instead of inlining a script after each component
@changeset-bot
Copy link

changeset-bot bot commented Apr 18, 2022

🦋 Changeset detected

Latest commit: 448cf57

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/solid-js Patch

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 pkg: solid Related to Solid (scope) pkg: integration Related to any renderer integration (scope) labels Apr 18, 2022
@hippotastic
Copy link
Contributor Author

Not sure what the issue with that failing CI test is. All tests pass locally for me (except the deno test, which always fails, even without my changes).

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Looks like a clean fix! I'm going to rerun the tests, might just be a flakey issue on Windows.

@natemoo-re natemoo-re merged commit 5e28b79 into withastro:main Apr 19, 2022
@github-actions github-actions bot mentioned this pull request Apr 19, 2022
@hippotastic hippotastic deleted the fix-solidjs-inline-hydration-script branch April 19, 2022 21:49
@ryansolid
Copy link
Contributor

ryansolid commented Apr 28, 2022

It's the weirdest thing but I can't place it. But the difference between the 0.1.1 and 0.1.2 of the Solid integration, which I believe is only this PR, consistently takes longer to server render. I don't see how this PR could be responsible but it is super consistent. No other packages changed.. just that. In my brutal Hackernews test I consistently see ~.7 second different FCP/LCP times.

If anyone has any ideas.

EDIT:

I have some ideas now. And it's completely annoying. It's incidental admittedly. It's that having all those script tags seemed to have interrupted the document getting it to paint part of the page earlier recording faster FCP and LCPs because it paints in chunks. Swapping to this more efficient way has it paint the whole page together.

I'm going to ignore this but it explains a lot.

Before:
Screen Shot 2022-04-28 at 4 23 28 PM

After:
Screen Shot 2022-04-28 at 4 24 00 PM

Look at where the FCP and LCP are in relation to the purple layout events (this is where it's doing the majority of the work)

@FredKSchott
Copy link
Member

Reminder that we should really bundle all of those scripts together into a single one on the page

@hippotastic
Copy link
Contributor Author

Reminder that we should really bundle all of those scripts together into a single one on the page

I agree, @FredKSchott, and this is exactly what my PR did. Surprisingly, the previous implementation that rendered tons of inline scripts had better FCP and LCP measurements in Ryan's extreme test case vs. mine that just uses a single script for the hydration.

Could you share the code of your Hackernews test so we could have a look, @ryansolid? Maybe we can find a solution to your discovery that the browser starts painting later when the document is not broken up into pieces through all the inline scripts anymore. At least I'd love to have a quick look.

@ryansolid
Copy link
Contributor

I think @FredKSchott is also referring to Astro's own scripts which you don't see in my screen shot but there are like 1000 of them after the document load event.

As for the painting thing, every other (non-streaming) implementation works the way this has been upgraded to. I'm not really concerned. Here's the repo, but I haven't updated, I was working on the edge branch but I needed to hack some things for it to work so haven't pushed. The behavior is equally applicable to the non-edge version: https://github.com/ryansolid/astro-solid-hackernews/tree/main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope) pkg: solid Related to Solid (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Solid integration renders script tags after each component instance, even non-hydrated ones
4 participants