Add client performance markers and measurements#1534
Add client performance markers and measurements#1534blittle wants to merge 2 commits intov1.x-2022-07from
Conversation
blittle
force-pushed
the
bl-add-client-perf-markers
branch
from
9f8cf50 to
8db4969
Compare
| if (rscReader) { | ||
| // The flight response was inlined during SSR, use it directly. | ||
| response = createFromReadableStream(rscReader); | ||
| const [streamForRsc, streamForMetrics] = rscReader.tee(); |
There was a problem hiding this comment.
I wish there was a better way to know when the stream is finished without tee-ing it.
There was a problem hiding this comment.
We are the ones creating this rscReader in this same file so we could just add a hook there, no? It's basically the same as DOMContentLoaded.
However, I don't think any of this is measuring the work that React does internally in createFromReadableStream since it's async and there are no hooks for that. We could add one and ask the React team to consider the use case, e.g. response.onDone = () => {}. I guess I'm not sure what part we're trying to measure here.
frandiox
left a comment
frandiox
left a comment
There was a problem hiding this comment.
Leaving a couple of comments here but I'm not sure if I understand the use-case.
Also, hydration vs rsc is kind of confusing to me. What do you think about using initial-hydration/ssr-hydration vs rsc/hydration
| performance.mark('--hydrogen-rsc-start'); | ||
|
|
||
| // Request a new flight response. | ||
| response = createFromFetch( | ||
| fetch(`${RSC_PATHNAME}?state=` + encodeURIComponent(key)) | ||
| ); | ||
| performance.mark('--hydrogen-rsc-end'); |
There was a problem hiding this comment.
Hmm, this all happens synchronously so I don't think there's a difference between rsc-start and rsc-end here, no?
There was a problem hiding this comment.
Looks like you are right. I had assumed that createFromFetch throw'ed.
| if (rscReader) { | ||
| // The flight response was inlined during SSR, use it directly. | ||
| response = createFromReadableStream(rscReader); | ||
| const [streamForRsc, streamForMetrics] = rscReader.tee(); |
There was a problem hiding this comment.
We are the ones creating this rscReader in this same file so we could just add a hook there, no? It's basically the same as DOMContentLoaded.
However, I don't think any of this is measuring the work that React does internally in createFromReadableStream since it's async and there are no hooks for that. We could add one and ask the React team to consider the use case, e.g. response.onDone = () => {}. I guess I'm not sure what part we're trying to measure here.
2 participants
Resolves: Shopify/hydrogen#1479
Before submitting the PR, please make sure you do the following:
fixes #123)yarn changeset addif this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning