One more idea for the profiler

[jerelmiller]

Take a look at this one @phryneas! If you like this, I'll go update all tests to match the new style.

---

There are a few additional improvements I want to make with the profiler so I'm proposing some additional changes to the API. The gist of the things I want to improve:

• Remove the ability to nest profilers in a single test render tree
• Make the creation of the component more reusable by decoupling it from the components it wraps.

Taking each one by one:

Remove the ability to nest profilers in a single test render tree

One of the goals of the profiler is to step through each render of the app and optionally test against a snapshot of that render, whether it be a DOM snapshot, or a snapshot on some data. By allowing the nesting of profilers, the timeline of snapshots for nested components gets hard to follow.

For example, take the following:

const App = profile({
  Commponent: () => {
    const [count, setCount] = useState(0);

    return (
      <>
        <button onClick={() => setCount(c => c + 1)>+</button>
        <Count count={count} />
      </>
    );
  }
})

const Count = profile({
  Component: ({ count }) => <div>{count}</div>
});

and my test:

render(<App />)

await user.click(screen.getByText('+'))
await user.click(screen.getByText('+'))
await user.click(screen.getByText('+'))

// initial render
await App.takeRender()
// first click
await App.takeRender()
// second click
await App.takeRender()

Here our test clicks the button three times, then steps through each render of <App />. Because we've interated our renders 3 times, we expect our next render to be the state at which the third button click happens. Since <Count /> is profiled however, its own render iterator is out-of-step with where we are in <App />, so calling Count.takeRender gives us the initial render, not the render at which we've stepped through with App. Its almost like our test is at two different points in time. In the long run, I think this has the potential to cause confusion and make our tests harder to follow.

This PRs aims to remove the ability to nest profilers and require that a single one is used on a particular subtree. This means that testing components should be done by stepping through the top-level profiler renders and checking subcomponents at each of those renders.

Make the creation of the component more reusable by decoupling it from the components it wraps.

In #11300, I avoid a lot of repetition in the setup of the tests by creating some "default" profiled components which track the renders of components such as the <SuspenseBoundary />. These components are extracted to a separate function outside the test which I can call to get those components. These components are created and tracked using profile, but now that I've removed the ability to nest profilers, I end up with a chicken-and-egg problem. Without the use of profile in those components, I need the profiled component to track data in the snapshot, but I can't create the profiled component until I'm able to get access to those component definitions.

Rather than having the profile function take a component and render it, instead I renamed this to createTestProfiler which returns a component that takes a children prop. This allows me to create the profiler first, then pass it as an argument to the test helper that creates my default profiled components.

By extracting it this way, this also makes it easier to create and use a more generic profiler that could be extracted to its own helper function for a set of tests that may share the same snapshot shape.

const Profiler = createTestProfiler()

// Profiler no longer requires `App` in order to work which means we can extract it and pass it around more easily.
render(<App />, { 
  wrapper: ({ children }) => (
    <Profiler>
      {children}
    </Profiler>
  )
})

[changeset-bot]

⚠️ No Changeset found

Latest commit: 4b9a0ec

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[jerelmiller]

This pattern makes it easier for me to extract this into a helper if need-be. In fact, 90% of the tests in this suite operate on the same snapshot shape, which means I could extract this to a reusable function to avoid some repetition in tests. For example:

function createLoadableQueryProfiler<TData>() {
  return createTestProfiler({
    initialSnapshot: {
      lastError: null as Error | null,
      result: null as UseReadQueryResult<TData> | null
    }
  })
}

then in my tests:

it('checks something about useLoadableQuery', async () => {
  const Profiler = createLoadableQueryProfiler();

  // snapshot is now of shape { lastError, result }
  const { snapshot } = await Profiler.takeRender()
})

[jerelmiller]

Rather than require profile to track a component, instead we allow components to opt into tracking by using the useTrackComponentRender hook. This hook now tracks the component function itself rather than needing to name it a specific way in the test, which feels a bit more natural:

const App = () => {
  useTrackComponentRender()
  // ...
}

const { renderedComponents } = await Profiler.takeRender()

expect(renderedComponents).toStrictEqual([App])

[phryneas]

Rather than require profile to track a component, instead we allow components to opt into tracking by using the useTrackComponentRender hook.

That's what the hook was designed to from the start.

We just had the wrapComponentWithTracking on top of it, to automatically wrap the profiled component, but the hook was always meant to be called from any component (and automatically determine it's name).

[jerelmiller]

Ya sorry thats what I meant, I just phrased it poorly. The sentence "require Profile to track a component" is not quite accurate and I don't even know why I put that there 🤷‍♂️

What I was getting at is that component tracking is now fully opt-in for every level of the component tree rather than having one always auto tracked. This can be useful if you're interested in checking against just a subcomponent renders without requiring that the top-level profiled component is also tracked.

This hook now tracks the component function itself

This was really one of the key things I was getting at here. Rather than:

expect(renderedComponents).toStrictEqual(['App', 'MyComponent'])

you can check against the component function itself:

const App = () => {

}

expect(renderedComponents).toStrictEqual([App])

[phryneas]

Ah, gotcha. I'd still like it if a string passed into the hook would be an option - that way you could render multiple instances of a component, but still distinguish between them by assigning a name for tracking:

expect(renderedComponents).toStrictEqual([App, 'UserProfile("Tom")', 'UserProfile("Jerry")'])

As for naming-vs-instance in general: I don't feel like it is "assigning a name specifically", since that's just how React component names work. But I'm fine either way.

[jerelmiller]

Now that we enforce a single profiler, it makes it more difficult to avoid stepping through each render like I originally did with the nested profiler approach.

[jerelmiller]

After converting a few tests to use the new patterns, I like the changes a lot. Seems like it strikes a nice balance between flexibility (profiler creation and reuse) and rigidity (enforce you to step through each render in your test). Unless there are any objections, I'd like to move forward with these changes.

[jerelmiller]

Don't worry about looking through all of these. I tried to convert a variety of use cases to ensure the updates to the API held up in each case.

[phryneas]

These components are created and tracked using profile, but now that I've removed the ability to nest profilers, I end up with a chicken-and-egg problem. Without the use of profile in those components, I need the profiled component to track data in the snapshot, but I can't create the profiled component until I'm able to get access to those component definitions.

I don't really see the problem tbh.?

This code should already be perfectly fine:

function SomeChild(){
    ProfiledApp.updateSnapshot(something)
    return <div>foo</div>
}

function App(){
    return <SomeChild />
}

const ProfiledApp = profile({ Component: App })

and the same way, this would be fine:

const ProfiledApp = profile({ Component: App })

function SomeChild(){
    ProfiledApp.updateSnapshot(something)
    return <div>foo</div>
}

function App(){
    return <SomeChild />
}

that said, I see that it might not necessarily be the "nicest" code.
The only problem would occur here if you used those in completely different scopes, but I think the suggested changes don't really address that, either.
I have to admit that the additional manual <Profiler> wrapping seems a bit complicated to me. 🤔

An alternative would be to completely decouple Profiler and component by using Context:

function SomeChild(){
    useProfiler().updateSnapshot(something)
    return <div>foo</div>
}

function App(){
    return <SomeChild />
}

const ProfiledApp = profile({ Component: App })

The downside of that would be that useProfiler would not be typesafe anymore, though.

To be honest, I might not 100% see the need for this change in the first place (I do agree on preventing the use of nested Profilers).
Could you go a little more into the original problem this tries to solve? I might be missing something here.

[phryneas]

Rather than require profile to track a component, instead we allow components to opt into tracking by using the useTrackComponentRender hook.

That's what the hook was designed to from the start.

We just had the wrapComponentWithTracking on top of it, to automatically wrap the profiled component, but the hook was always meant to be called from any component (and automatically determine it's name).

[jerelmiller]

I don't really see the problem tbh.?

Ya I could have definitely been a bit better about fully explaining this. I was trying to rush this as it was almost dinner. Ooops.

I take back what I said about the "chicken-and-egg" problem. Again, rushing too fast I didn't think through what I was saying and what I was getting at wouldn't actually be an issue in practice, so ignore this point.

---

Let me start over and see if I can describe this a bit better. Apologies ahead of time, this might be a bit theoretical.

With the way profile is setup today, which takes a component as input, its API implies we are trying to profile a single component and get some data about it.

Instead I see the profiler as a means to measure and track information against an entire React subtree, which is especially apparent with the way useBackgroundQuery and useLoadableQuery tests are created since we have several components we are trying to track against (parent, child, and suspense boundary). Because of this, it felt like the profiler itself should be decoupled from a single component and instead be used to wrap a component subtree, hence the change to the API:

const Profiler = createTestProfiler()

render(
  <Profiler>
    <SubtreeIWantToMeasure />
  </Profiler>
)

I felt this API communicated this a bit better.

This change also means we can test sibling components together if we'd like:

<Profiler>
  <AComponentIWantToTrack />
  <AnotherComponentIWantToTrack />
</Profiler>

This is probably not a common use case, but it is certainly possible and benefits from the fact that it wouldn't need a wrapping component just to render both of these together as siblings.

---

My original idea on the decoupling here though was the fact that I wanted a higher degree of reusability on the profiler itself. It feels more natural to create it first, then use it any component I wanted. This means I could extract a profiler with a common setup to its own reusable function and call that in each test without knowing what component or subtree it would operate on first.

Take this code with the inverse approach:

function createDefaultTrackedComponents(profiler) {
  function ErrorFallback({ error }) {
    profiler.mergeSnapshot({ error })
  }
}

test('it tests something', async () => {
  function App() {
    Profiler.mergeSnapshot({ someData: true })

    return <ErrorBoundary fallback={<ErrorFallback />} />
  }

  const ProfiledApp = profile({ Component: App })
  const { ErrorFallback } = createDefaultTrackedConponents(ProfiledApp)
})

While this works since ProfiledApp and ErrorFallback are used inside the function body, its always felt a little funny to me declare these in reverse order. With the existing profile API though, creating some reusable setup code that needs access to the profiler, I'd have to declare my tests this way in order to work.

This was what I was initially getting at with "chicken-and-egg". App depends on the profiler, but I can't create the profiler until I have the App. Similarly, the App needs to render one of those reusable components, but that reusable component needs the profiler to be created first before it can be used inside its function body.

Again, this is less of a technical limitation here, more of just a bit of a mental hurdle (at least for me) to declare things in this order. The ability to reuse some components that I otherwise end up copying over and over for each test is useful in a setup function I can just call.

I hope this makes a bit more sense.

[phryneas]

Makes more sense now, especially in complexer scenarios.

Suggestion: What do you think about making Component optional and using

Component = ({ children }) => {
  if (!children) throw new Error("needs to be used as a wrapper!");
  return children;
};

as default value?

That would enable this api for complex use cases, (keep in mind that Profiler should be part of the wrapper option, not the rendered app itself, or you can't rerender it with new props though!), but also keep the existing api for simple use cases where the wrapping would just add complexity to the test.

[jerelmiller]

I like that idea of making Component optional that way you get the best of both worlds! Appreciate the suggestion.

keep in mind that Profiler should be part of the wrapper option

Ah yes thanks. I keep forgetting to do this for some reason. Appreciate it!

[jerelmiller]

@phryneas alright I've resurrected profile which is now implemented in terms of createProfiler. I think this is a good "best of both worlds" situtation: 0d141df.

This means existing tests can be left alone: 4b9a0ec. I'll go ahead and get this merged and get the rest of the useLoadableQuery tests updated to match the changes here. Thanks a bunch for working through this with me!

[jerelmiller]

Merging and finishing the work over in #11300