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

feat: Send RSC Flight Payload to Studio #10213

Merged
merged 15 commits into from
Mar 16, 2024
Merged

Conversation

dthyresson
Copy link
Contributor

@dthyresson dthyresson commented Mar 12, 2024

This PR sends the rendered RSC payload (aka "flight") to Studio to be ingested, persisted, and visualized. See: redwoodjs/studio#52

Am encoding the payload as base64 text as a convenience so it gets persisted as encoded text an not some unwieldily JSON with escape characters. Studio will decoded and handle the flight payload accordingly.

This is a proof of concept and still many todos:

  1. Ensure only sends in dev and studio setup
  2. Could do some refactoring to separate enrichment

Studio

image

and single preview for the moment ... still UI todo

image

@dthyresson dthyresson self-assigned this Mar 12, 2024
@dthyresson dthyresson added the release:feature This PR introduces a new feature label Mar 12, 2024
@dthyresson dthyresson marked this pull request as ready for review March 15, 2024 15:00
@dthyresson dthyresson changed the title DRAFT: feat: Proof of Concept to Send RSC Flight Payload to Studio feat: Send RSC Flight Payload to Studio Mar 15, 2024
@dthyresson dthyresson added this to the RSC milestone Mar 16, 2024
@dthyresson
Copy link
Contributor Author

@Tobbe Probably good to get this reviewed and any refactor ideas since where the send to Studio is added is at the end of a rather long function; so, open to suggestions.

@dthyresson dthyresson requested a review from Tobbe March 16, 2024 09:39
@dthyresson
Copy link
Contributor Author

dthyresson commented Mar 16, 2024

@Tobbe One question I had is if I should check the toml config for the Studio options -- and use that as a conditional to send or not to send to Studio? Or should we have an additional setting -- and if so, which?

.changesets/four-adults-jog.md Outdated Show resolved Hide resolved
packages/vite/src/rsc/rscStudioHandlers.ts Outdated Show resolved Hide resolved
@Tobbe
Copy link
Member

Tobbe commented Mar 16, 2024

@dthyresson Can you please fix the linting errors and then I'll have another look

@dthyresson
Copy link
Contributor Author

I just noticed those -- will do.

dthyresson and others added 3 commits March 16, 2024 10:50
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@dthyresson
Copy link
Contributor Author

@dthyresson Can you please fix the linting errors and then I'll have another look

I had to fix the linter comma issues with yarn lint --fix.

On saving the file, my prettier config seems to remove them. Perhaps my prettier is out of date?

@dthyresson
Copy link
Contributor Author

dthyresson commented Mar 16, 2024

Also, I imagine I should not send to Studio during CI or test -- so, will want a way to disable this as well in those cases.

Although: yarn test did pass locally.

@dthyresson dthyresson requested a review from Tobbe March 16, 2024 10:22
Comment on lines 131 to 136
const startedAt = Date.now()
const start = performance.now()
const pipeable = await renderRsc({ rscId, props, rsfId, args })
const endedAt = Date.now()
const end = performance.now()
const duration = end - start
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of interest was it easier to use these different .now functions rather than having startAt come from the performance.now? Is performance.now difficult to turn back into a JS Date object? Just interested, don't mean to be nitpicker or anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to track start and end date times (proper timestamps) so startedAt, endedAt.

The performance start and end are some msec since Node starts something, so just used for the duration.

In the metadata I send to Studio, I send the startedAt, endedAt and duration:

      performance: {
        startedAt,
        endedAt,
        duration,
      },

but not those other time values -- because, yes, near impossible to transform back into date times.

@dthyresson
Copy link
Contributor Author

@Tobbe I refactored to get the Studio port from the config and only send to Studio when needed.

Note that I have it sending in production now as apps run in production -- so need this for testing and demos.

I also render RSC a second time just for Studio so I can wrap that rendering in performance only for Studio and never in production.

This may add a tiny perf fit when in Dev with Studio, but seems reasonable to not add those perf checks in production?

@dthyresson dthyresson requested a review from Tobbe March 16, 2024 13:46
@dthyresson
Copy link
Contributor Author

@Tobbe I made the logic change to isDevelopement and shouldSendToStudio. Makes more sense here. Thanks!

const shouldSendToStudio = () => {
  // TODO (RSC): This should be just isDevelopment()
  // but since RSC apps currently run in production mode
  // we need to check for 'production' (aka not 'development')
  // for now when sending to Studio
  return isStudioEnabled() && !isDevelopment()
}
image image

@dthyresson dthyresson merged commit 18a55d4 into main Mar 16, 2024
44 checks passed
@dthyresson dthyresson deleted the dt-rsc-flight-preview-poc branch March 16, 2024 15:26
jtoar pushed a commit that referenced this pull request Mar 20, 2024
At the time of #10213 the changeset CLI utility generated the name as a
random three-word combo. This was later updated to be the PR number
instead. This PR renames that one older entry so the whole folder has
the same naming convention. Getting a review on this to ensure this
won't break some tooling/state that I'm not as familiar with.
thedavidprice pushed a commit that referenced this pull request Mar 27, 2024
At the time of #10213 the changeset CLI utility generated the name as a
random three-word combo. This was later updated to be the PR number
instead. This PR renames that one older entry so the whole folder has
the same naming convention. Getting a review on this to ensure this
won't break some tooling/state that I'm not as familiar with.
@Josh-Walker-GM Josh-Walker-GM modified the milestones: RSC, v8.0.0 Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants