Add dangerous_disableHookValueTransportation to ManualDataTransport
#463
Conversation
🦋 Changeset detectedLatest commit: d70da04 The changes in this PR will be included in the next version bump. 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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
✅ Deploy Preview for apollo-client-nextjs-docmodel ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
@apollo/client-react-streaming
@apollo/experimental-nextjs-app-support
@apollo/client-integration-nextjs
@apollo/client-integration-react-router
@apollo/client-integration-tanstack-start
commit: |
size-limit report 📦
|
|
Hello @phryneas! First off, thank you so much for all of the incredible work across the Apollo ecosystem. I was responsible for migrating a large code base from pages router to app router and you and the other maintainers' work was an absolute life-saver! I've recently come to notice how much bandwidth the hook serialization is consuming in our applications and I'm very interested in exploring ways to cut back on that, especially considering our applications aren't massively interactive and most of the state for that interactivity is outside the Apollo Cache. WIth that said, I'm also still somewhat wary of turning off the hook synchronization entirely. Would you be open to a more granular approach? Possibly opting types/fields in/out of hook serialization or opting specific calls to hooks out of the serialization? If one of these sounds like a good approach to you I'd be happy to put together a more concrete API proposal and potentially a PR. |
|
On a lighter-weight front, could we get this merged into the library with a name that makes it clear that it's experimental? It seems like the hardest part about getting some testing behind this would be applying these changes. We're specifically using the NextJS integration so having it surface there as an experimental feature as well would be ideal. |
|
@TomMahle you can always install the published version of the package with this PR in it from this comment: npm i https://pkg.pr.new/apollographql/apollo-client-integrations/@apollo/client-integration-nextjs@463As for making this even more configurable - generally I'd be open to that, but first I'd like some real-life data that this
|
apollo-client-nextjs#565 Bundle Size — 1.29MiB (~+0.01%).d70da04(current) vs c465aff main#562(baseline) Warning Bundle contains 1 duplicate package – View duplicate packages Bundle metrics
|
| Current #565 |
Baseline #562 |
|
|---|---|---|
 Initial JS |
1.03MiB(~+0.01%) |
1.03MiB |
 Initial CSS |
0B |
0B |
 Cache Invalidation |
6.97% |
0.08% |
| Chunks |
35 |
35 |
| Assets |
59 |
59 |
| Modules |
641 |
641 |
| Duplicate Modules |
88 |
88 |
| Duplicate Code |
4.08% |
4.08% |
| Packages |
25 |
25 |
| Duplicate Packages |
1 |
1 |
Bundle size by type 1 change 1 regression
| Current #565 |
Baseline #562 |
|
|---|---|---|
| JS |
1.28MiB (~+0.01%) |
1.28MiB |
| Other |
9.59KiB |
9.59KiB |
Bundle analysis report Branch pr/disableHookValueTransportatio... Project dashboard
Generated by RelativeCI Documentation Report issue
|
I did some local testing of this today with a few use cases on a large and heavily streamed web app and saw HTML resource size reduce by 36%, 61%, and 44% with transferred size reducing by 24%, 61%, and 17%. These would represent substantial performance improvements for this application! Here are those HTML document sizes expressed as [kB over the wire] / [kB resource size] for a few pages in one of our apps.
In terms of hydration errors I could not find any difference between hook transport enabled vs. disabled though I'll note that I'm working off of next 14 for all of these results and hitting hydration mismatches even outside Apollo state were pretty easy to reproduce with interaction while streaming. [Edit]: I should have known better than to promise anything about timing 😅 My day was eaten up by other concerns, as we all know happens often. ⏳ |
|
Apologies for the delay, it turns out upgrading across major versions of core dependencies in an enterprise app containing several legacy components took a while 😅. The good(?) news is that in local development and low-speed network environments the pages I work on already have easy-to-reproduce hydration errors 🫤 that are, for our purposes, identical to what would be risked by turning on this option and finishing up porting our state management into Apollo. Furthermore, these experiences have had those errors in production on a high-traffic site (akarank top 10000) for over a year 😮💨. These are holdovers from a state-management paradigm that worked fine in pages router but has fundamental flaws in App router that we didn't know about until I was testing this option because no one has reported issues! Undoubtedly, our logging and/or monitoring needs some work, but I want to focus on that whole 'no one has reported issues even with hydration errors' side of things 👍. This is because even on React 18 and Next 14 that we have in production these do not have catastrophic consequences for UX 😌. You can see that here or here by throttling network and interacting with pickup/shipping on the right side. So, while I don't have hard data for you on how many hydration errors this option is likely to cause I can still say confidently that this option is worth shipping, perhaps still with an I still think there's value in providing opt-in / opt-out capabilities on a field/type/hook-invocation basis and am happy to continue on with that conversation, but that doesn't seem to interfere with this option of specifying a default behavior. Can we get this shipped? 😁 |
|
For posterity I'm drawing a connection from here back to #344 which is what originally got me looking down this path. |
|
For context: I'm running into a bit of a chicken-and-egg scenario where there is hesitation around utilizing this functionality in production unless it is merged into main and thus guaranteed to have an up-to-date version containing it. If we can get this merged in to main, even with something identifying it as experimental, I'll have a much easier time getting it used in prod and reporting back whatever specific metrics are desired 😄. Hopefully my findings above are enough to get past that hurdle. |
|
@phryneas Hello again! Where would you like to go from here with this? If you've just been busy or relaxing, no worries. If there's something else specific we need in order to get this merged in let's talk about what that looks like. I'd really like to get this change out to our users, I just need a way to make sure we can get them any upstream changes to this library easily and punctually as well. |
|
Hi @TomMahle, sorry for the delay in responding. Once that's done - ETA is in 2 weeks - I'll get around to all the surrounding problems and will pick this up, too - I hope that's okay! |
@phryneas Thanks for getting back to me! That should work fine. If I could get access to be able to merge main into this PR or assurance that this will be kept up to date in case of any unexpected high priority main merges I may be able to get this out in front of some users. |
|
@TomMahle you know what? Let's just get this in in-between. I'll await for a review from @jerelmiller, but from my side tbh. this is as ready as it can be. |
packages/client-react-streaming/src/ManualDataTransport/ManualDataTransport.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Jerel Miller <[email protected]>
phryneas
changed the title
disableHookValueTransportationoption to ManualDataTransportdangerous_disableHookValueTransportation to ManualDataTransport
|
@phryneas Thank you very much! |
We would need feedback from someone who runs this for a while with real traffic before we can consider adding this to the library.