-
Notifications
You must be signed in to change notification settings - Fork 220
feat: allow adding lazy loaded scripts to head to avoid hydration errors #2802
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Size Change: +1.17 kB (+0.02%) Total Size: 5.83 MB
ℹ️ View Unchanged
|
375a514 to
c19e9e8
Compare
c19e9e8 to
a249b2e
Compare
paperclover
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you. i spent a long time thinking if this was the solution or not, but wasn't fully confident in my thinking.
rafaeelaudibert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, should be the same! This is a breaking change in a way, any chance we could hide it behind a new defaultsor do you feel positive this is good to go?
|
i was thinking not breaking because there should be no action on users as a result i'll wait till the new year and deploy when i an concentrate on it i do think it'd be nice to get the SDKs in to the mono-repo so we could validate JS SDK in the app without releasing it maybe (and then keep posthog.com for validating the html snippet works) but that's not a quick thing so 🙈 |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
As Tanstack Start doesn't have yet an official posthog integration, this will greatly simplify current setup :) thanks! |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
@pauldambra the new year has come and gone, is there anything stopping this from being merged? |
a249b2e to
cf21f59
Compare
cf21f59 to
606f619
Compare
606f619 to
9f4d2a4
Compare
|
@rafaeelaudibert i decided to be a good citizen and add this to a new default it's a low chance but someone could be bound to the existing behaviour in some unexpected way i'll merge tomorrow when i'm around to pay attention |

avoid hydration errors #2781
when someone is using SSR and we're lazy loading scripts we have a race between react hydrating and us altering the DOM
if we manage to alter the DOM before hydration complests then we'll cause a hydration error
i don't believe we care where the script is added (since we're adding it async from code the location isn't going to affect whether it blocks) so we can add the script to the head and not have any risk of causing a hydration error
Libraries affected