Skip to content

fix: Error: Cannot read property 'duration' of undefined#18806

Closed
Amine-H wants to merge 3 commits into
react:masterfrom
Amine-H:fix/18786
Closed

fix: Error: Cannot read property 'duration' of undefined#18806
Amine-H wants to merge 3 commits into
react:masterfrom
Amine-H:fix/18786

Conversation

@Amine-H

@Amine-H Amine-H commented May 3, 2020

Copy link
Copy Markdown

Summary

Fix for #18786 and #18798

Just a few things I should mention first, I am really new to the code base, not sure what was the intention to use the indexes from commits instead of commitData.
In other words if commitData has more commit than commits (interaction commits) then this PR would be showing more commits than whan It should.

UPDATE:

I have updated my changes to "ignore" commits that have no corresponding commitData

Test Plan

Thanks @shiftyp for the clear reproduction steps

@codesandbox-ci

codesandbox-ci Bot commented May 3, 2020

Copy link
Copy Markdown

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 3c6ea8f:

Sandbox Source
blissful-field-ej3kj Configuration

@sizebot

sizebot commented May 3, 2020

Copy link
Copy Markdown

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 3c6ea8f

@sizebot

sizebot commented May 3, 2020

Copy link
Copy Markdown

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 3c6ea8f

@Amine-H

Amine-H commented May 3, 2020

Copy link
Copy Markdown
Author

After further investigating the issue, I have pushed some changes that would "ignore" commits that have no corresponding commitData

I found out that the changes I pushed eaerlier aren't a valid fix, so I canceled them.

the issue is caused when you stop profiling while the page still doing some work. causing either:

  • missing commit in commitData
  • interactionCommits has an unexisting commitIndex

@bvaughn bvaughn self-requested a review May 4, 2020 16:12
@bvaughn

bvaughn commented May 7, 2020

Copy link
Copy Markdown
Contributor

Thanks for submitting this PR!

I'm a little concerned that while it fixes a symptom (out of range index) it doesn't fix the underlying cause (why is the data missing?)

Adding some logging to DevTools and trying the repro, I can see that e.g. the backend records 181 commits but then the later profiling data only contains 180 commits. So one of them is being filtered out for some reason or is not being sent across the bridge. Either way this is likely to cause bigger problems than just this specific fix, so I'm going to look into it further.

Edit 1 All of the commits are being sent over the bridge to the frontend, but it seems to be filtered from the data that's exported.

Edit 2 I think this might be what's biting us:

https://github.com/facebook/react/blob/fb3f0acad9ac0b5756724d53eddaa444767dca07/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js#L66-L73

Edit 3 Yup, that was it. At least for this specific case, we're filtering out a commit where "nothing renders". I think this could happen if the only things that rendered were also hidden by our component filters...

Screen Shot 2020-05-07 at 3 09 42 PM

Edit 4 Confirmed. Changing the filters to intentionally hide all of the components that would be part of a commit does repro this same error behavior. So that's fine. The fix, then, is to just remove this filter step!

@bvaughn

bvaughn commented May 7, 2020

Copy link
Copy Markdown
Contributor

Going to close this PR in favor of #18862!

@bvaughn bvaughn closed this May 7, 2020
@Amine-H Amine-H deleted the fix/18786 branch May 7, 2020 22:29
@Amine-H

Amine-H commented May 7, 2020

Copy link
Copy Markdown
Author

Alright ! that should do it! thanks 👍

@bvaughn

bvaughn commented May 7, 2020

Copy link
Copy Markdown
Contributor

No problem! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants