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

Doesn't support profiler even with latest React version #16685

Closed
iamgbayer opened this issue Sep 6, 2019 · 10 comments
Closed

Doesn't support profiler even with latest React version #16685

iamgbayer opened this issue Sep 6, 2019 · 10 comments

Comments

@iamgbayer
Copy link

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
The profiler tab only shows this message even though I have a version greater than 16.5.

Screen Shot 2019-09-06 at 13 26 47

Something strange on the console is this message and just appears when the extension is active.

Screen Shot 2019-09-06 at 13 25 17

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
React 16.9.0
React DevTools 4.0.6-a39d9c3

@bvaughn
Copy link
Contributor

bvaughn commented Sep 6, 2019

The profiler tab only shows this message even though I have a version greater than 16.5.

Profiling requires either a DEV build or the production+profiling build. Are you running one of these?

@iamgbayer
Copy link
Author

I tested with two applications one using CRA and another using a custom Webpack config, both running on DEV build

Both return the same error.

@threepointone
Copy link
Contributor

Could you make a git repo that demonstrates this problem? It sounds like a configuration problem, but it's hard to comment without seeing the actual project.

@bvaughn
Copy link
Contributor

bvaughn commented Sep 9, 2019

Yeah. We've tested a lot on this end, with several common setups (including CRA), and it works - so without having more info from your end on how to repro this, it's hard to help.

Just tested again to double check:

create-react-app test-profiler
cd test-profiler
yarn start

Profiler works fine. The issue template asks:

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Could you clarify which OS, browser, and browser versions you're using please? The runtime error you mention makes me wonder if this is an unsupported browser.

@iamgbayer
Copy link
Author

iamgbayer commented Sep 11, 2019

I tested with this repo below using CRA, but I think that be something in the application I'm doing wrong, because of testing only with create-react-app test-profiler and starting the application, there was no problem.

This repo has the issue @threepointone, https://github.com/rentinga/client

I'm using an OSX High Sierra with Chrome 76.

@LetItRock
Copy link
Contributor

LetItRock commented Sep 14, 2019

Hi! @bvaughn @iamgbayer

I've found what was causing an issue here. In the sample application there is a code which does this:
Screenshot 2019-09-14 at 17 05 17

When the DevTools hook is called with Fiber Tree it records all fibers in react-devtools-shared renderer.js. While doing this it takes displayName from fiber node calling getDisplayName function from utils.js. The function looks for displayName on fiber node or if there is no displayName then it takes name property which is in this case - an object.

Screenshot 2019-09-14 at 17 28 47

Later on, it tries to generate string id using that object in getStringId function:
Screenshot 2019-09-14 at 17 02 57

Which sets variable pendingStringTableLength with NaN, because object.length is undefined and if you add any number to it you will get NaN. As it keeps adding numbers - the value doesn't change from NaN.
After recursively recording all the fiber nodes flushPendingEvents function is called which tries to create operations array with pendingStringTableLength variable which is still NaN causing new Array(NaN) to throw "Uncaught RangeError: Invalid array length" error.


I thought that the fix for this could be doing the same check as for type.displayName in getDisplayName function, like to check if typeof type.name === 'string' and if it's true - use that name, otherwise take fallback name. But I'm not really familiar with the code here, WDYT @bvaughn?


@iamgbayer If you will remove that Icon.name = Object.assign... code it will work :)

@bvaughn
Copy link
Contributor

bvaughn commented Sep 15, 2019

Great sleuthing, @LetItRock!

typeof type.name === 'string' check sounds like a good approach to me. Interested in contributing a PR? A unit test would be fantastic too.

@LetItRock
Copy link
Contributor

Thanks, sure :)

@iamgbayer
Copy link
Author

Great research @LetItRock, thank you 🎉

I based myself on this approach to expose the possible props, https://design-system.pluralsight.com/components/avatar

Thanks @bvaughn and @threepointone for the orientation to find the problem 🎉

@bvaughn
Copy link
Contributor

bvaughn commented Sep 17, 2019

Now that #16798 has landed, this issue should be fixed with the next DevTools release.

@bvaughn bvaughn closed this as completed Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants