Update DevTools to use getCacheForType API

[bvaughn]

DevTools was built with a fork of an early idea for how Suspense cache might work. This idea is incompatible with newer APIs like useTransition which unfortunately prevented me from making certain UX improvements. This PR swaps out the primary usage of this cache (there are a few) in favor of the newer unstable_getCacheForType and unstable_useCacheRefresh APIs. We can go back and update the others in follow up PRs.

Messaging changes

I've refactored the way the frontend loads component props/state/etc to hopefully make it better match the Suspense+cache model. Doing this gave up some of the small optimizations I'd added but hopefully the actual performance impact of that is minor and the overall ergonomic improvements of working with the cache API make this worth it.

The backend no longer remembers inspected paths. Instead, the frontend sends them every time and the backend sends a response with those paths. I've also added a new "force" parameter that the frontend can use to tell the backend to send a response even if the component hasn't rendered since the last time it asked. (This is used to get data for newly inspected paths.)

Initial inspection...

front |                                                      | back
      | -- "inspect" (id:1, paths:[], force:true) ---------> |
      | <------------------------ "inspected" (full-data) -- |

1 second passes with no updates...

      | -- "inspect" (id:1, paths:[], force:false) --------> |
      | <------------------------ "inspected" (no-change) -- |

User clicks to expand a path, aka hydrate...

      | -- "inspect" (id:1, paths:['foo'], force:true) ----> |
      | <------------------------ "inspected" (full-data) -- |

1 second passes during which there is an update...

      | -- "inspect" (id:1, paths:['foo'], force:false) ---> |
      | <----------------- "inspectedElement" (full-data) -- |

Clear errors/warnings transition

Previously this meant there would be a delay after clicking the "clear" button. The UX after this change is much nicer:

Hydrating paths transition

I also added a transition to hydration (expanding "dehyrated" paths):

Better error boundaries

I also added a lower-level error boundary in case the new suspense operation ever failed. It provides a better "retry" mechanism (select a new element) so DevTools doesn't become entirely useful. Here I'm intentionally causing an error every time I select an element:

Improved snapshot tests

I also migrated several of the existing snapshot tests to use inline snapshots and added a new serializer for dehydrated props. Inline snapshots are easier to verify and maintain and the new serializer means dehydrated props will be formatted in a way that makes sense rather than being empty (in external snapshots) or super verbose (default inline snapshot format).

Before (in __snapshots__/inspectedElementContext-test.js.snap)

After (inline)

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cf267fe:

Sandbox	Source
React	Configuration

[sizebot]

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against cf267fe

[sizebot]

Details of bundled changes.

Comparing: 95feb0e...cf267fe

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	+2.7%	+2.6%	617.67 KB	634.21 KB	130.3 KB	133.71 KB	UMD_DEV
react-test-renderer.production.min.js	🔺+3.0%	🔺+3.5%	77.74 KB	80.07 KB	24.43 KB	25.3 KB	UMD_PROD
ReactTestRenderer-profiling.js	0.0%	0.0%	245.03 KB	245.03 KB	44.84 KB	44.84 KB	RN_FB_PROFILING
react-test-renderer.development.js	+2.7%	+2.6%	588.41 KB	604.12 KB	128.76 KB	132.08 KB	NODE_DEV
react-test-renderer.production.min.js	🔺+3.0%	🔺+3.3%	77.55 KB	79.88 KB	24.13 KB	24.92 KB	NODE_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against cf267fe

[bvaughn]

Tests were failing b'c we never enabled cache for experimental test renderer builds (so the hook was missing).

[bvaughn]

Cleaned this up a bit. My component organization here is still a little janky (e.g. the reason I have the memo-busting counter). I could refactor to move the context lower, within the suspense boundary, so that I could just cache on the object itself, but I noticed a lingering update that never completes (just re-renders at an interval) the first time I use startTransition to clear a warning.

Maybe I should do a more heavy refactor and try to remove this context entirely. Something needs to poll for updates and refresh the cache but it doesn't necessarily need to be a context provider that sits above the Suspense boundary.

I may still be missing something about how the pieces for this kind of cache fit together. It's more complex than our fs/db cache implementations, since there are a couple of cases for invalidation (polled update from the backend, hydrated path, user clicked to clear errors/warnings). Normally I would refer to other places we're using the API but it seems like our useCacheRefresh API hasn't really been used for anything yet, outside of a couple of tests.

[bvaughn]

I've refactored the way the frontend loads component props/state/etc to hopefully make it better match the Suspense+cache model. Doing this gave up some of the small optimizations I'd added but hopefully the actual performance impact of that is minor and the overall ergonomic improvements of working with the cache API make this worth it.

The backend no longer remembers inspected paths. Instead, the frontend sends them every time and the backend sends a response with those paths. I've also added a new "force" parameter that the frontend can use to tell the backend to send a response even if the component hasn't rendered since the last time it asked. (This is used to get data for newly inspected paths.)

Here's an example scenario:

Initial inspection...

front |                                                      | back
      | -- "inspect" (id:1, paths:[], force:true) ---------> |
      | <------------------------ "inspected" (full-data) -- |

1 second passes with no updates...

      | -- "inspect" (id:1, paths:[], force:false) --------> |
      | <------------------------ "inspected" (no-change) -- |

User clicks to expand a path, aka hydrate...

      | -- "inspect" (id:1, paths:['foo'], force:true) ----> |
      | <------------------------ "inspected" (full-data) -- |

1 second passes during which there is an update...

      | -- "inspect" (id:1, paths:['foo'], force:false) ---> |
      | <----------------- "inspectedElement" (full-data) -- |

Note that CI is expected to fail at the moment because I have several tests to sort out still. Actual DevTools seems to be working though based on my testing.

[bvaughn]

All tests pass now except for "should poll for updates for the currently selected element" (in inspectedElement-test). That one's interesting because it seems like it should be passing, except the transition+refresh that get scheduled never gets rendered. Will dig in more this afternoon.

[bvaughn]

The only test that's failing now is this one:

it('should poll for updates for the currently selected element', async done => {
  const Example = () => null;

  const container = document.createElement('div');
  await utils.actAsync(
    () => ReactDOM.render(<Example a={1} b="abc" />, container),
    false,
  );

  const id = ((store.getElementIDAtIndex(0): any): number);

  let inspectedElement = null;

  function Suspender({target}) {
    inspectedElement = useInspectedElement(id);
    return null;
  }

  let renderer;

  await utils.actAsync(() => {
    renderer = TestRenderer.create(
      <Contexts defaultSelectedElementID={id} defaultSelectedElementIndex={0}>
        <React.Suspense fallback={null}>
          <Suspender target={id} />
        </React.Suspense>
      </Contexts>,
    );
  }, false);
  expect(inspectedElement.props).toMatchInlineSnapshot(`
    Object {
      "a": 1,
      "b": "abc",
    }
  `);

  await utils.actAsync(
    () => ReactDOM.render(<Example a={2} b="def" />, container),
    false,
  );

  // Wait for our check-for-updates poll to get the new data.
  jest.runOnlyPendingTimers();
  await Promise.resolve();

  expect(inspectedElement.props).toMatchInlineSnapshot(`
    Object {
      "a": 2,
      "b": "def",
    }
  `);

  done();
});

The bit that waits for the check-for-updates poll works, in that– new data comes in and I refresh the cache, but that update never goes anywhere. React doesn't re-render <Suspender> afterward and so my test fails.

Interestingly the test passes if I include both of the following after the ReactDOM update render:

// Wait for our check-for-updates poll to get the new data.
jest.runOnlyPendingTimers();
await Promise.resolve();

await utils.actAsync(
  () =>
    renderer.update(
      <Contexts
        defaultSelectedElementID={id}
        defaultSelectedElementIndex={0}>
        <React.Suspense fallback={null}>
          <Suspender target={id} />
        </React.Suspense>
      </Contexts>,
    ),
  false,
);

But if I only include one of the two, the test fails. It seems like either one should be sufficient by itself.

[bvaughn]

Okay. That test is still not great and I'd love to get input on how to resolve it. Otherwise I think this PR is ready for another set of eyes!

[acdlite]

Neat! I only reviewed the part directly related to the cache API. I don't think I can comment on the rest of it because I don't know the DevTools codebase that well. Lemme know if you want a more thorough review.

[eps1lon]

That test is still not great and I'd love to get input on how to resolve it.

@bvaughn Does #20618 help?

[bvaughn]

Thanks @acdlite! I wish I knew why that one test required such approach to pass, but I think that's something I can follow up with later.

[bvaughn]

That test is still not great and I'd love to get input on how to resolve it.

@bvaughn Does #20618 help?

@eps1lon That comment was referring to this which is different.

Your findings with the inline snapshot are interesting though. So the real issue was the async -> done() callback? (Obviously the date formatting issue but that's separate.) Maybe we can discuss the pros and cons of that in a follow up PR :)