fix(devtools): check if data for commit index exists

[jyash97]

Summary

After Reload and profiling this app: https://gsqzl.csb.app/, traverse through the commit. Devtools break and throw an error for accessing data from undefined or null.

Profiling Data: https://gist.github.com/jyash97/612bc8d38c2f7f9842c19ae5d270252c

For more ref: #18033 (comment)

Test Plan

Added the check if the commit data exists than only access the value.

Tested with firefox, chrome extension and with inline package.

Commands used

yarn build:chrome && yarn test:chrome

&

yarn start ( for inline & shell package )

Screenshot Recording of Chrome extension: https://youtu.be/1UyEZcfbjqg

[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 466d0ed:

Sandbox	Source
zealous-dan-xfzeh	Configuration

[sizebot]

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 466d0ed

[sizebot]

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 466d0ed

[jyash97]

@bvaughn Do check these when you get time and lmk if the PR makes sense.

[bvaughn]

@jyash97 Nice repro! Can you share the source for the repro app?

[bvaughn]

I think we should find and fix the underlying cause here. Why is operations[commitIndex] undefined? That's unexpected.

[jyash97]

On debugging the underlying issue, I encountered this issue so I thought it would be better to add the check and get the correct error.

[jyash97]

Hey @bvaughn thanks for taking a look.
Here id the Sandbox Source: https://codesandbox.io/s/getcommittree-o29bx

[bvaughn]

There is a duplicate key warning in this app:

Warning: Encountered two children with the same key, `1`.
Keys should be unique so that components maintain their identity across updates.
Non-unique keys may cause children to be duplicated and/or omitted —
the behavior is unsupported and could change in a future version.
    in Suspense (at App.js:30)
    in header (at App.js:29)
    in div (at App.js:28)
    in App (at src/index.js:7)

I don't understand why it manifests the way it does with the Profiler, but if you fix that warning- the behavior of your app is what I would expect (and the Profiler works fine). (See here.)

[bvaughn]

I've reduced the error case to poke around a little further though, to see if I can figure out why the Profiler is reacting the way it is. (See here.)

const One = () => null;
const Two = () => null;

function App() {
  const [didMount, setDidMount] = useState(false);

  useEffect(() => {
    setDidMount(true);
  }, []);

  return (
    <>
      {didMount ? <Two /> : <One key={1} />}
      <One key={1} />
    </>
  );
}

[bvaughn]

Ah, I see why the Profiler is erroring in this case. The invalid key situation causes a DevTools invariant:

react/packages/react-devtools-shared/src/devtools/store.js

Lines 929 to 933 in 0140118

	if (children.length !== numChildren) {
	throw Error(
	`Children cannot be added or removed during a reorder operation.`,
	);
	}

This thrown error means that the Profiler code that normally processes the commit (after the Components store) doesn't run.

I may update this specifically, but in general- this case isn't really meant to be supported. I wish we "failed" in a more meaningful way though! Let me give it a little thought.

[bvaughn]

I've opened an alternate "fix" for this: #18378

Want to check it out?

[jyash97]

Hey @bvaughn thanks for the review and letting me know the underneath issue. I will surely take look at your PR 💯