Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Switched getExtraProps to use findClosestFunction to get the component name. - #5851

Merged
jasonLaster merged 3 commits into
firefox-devtools:masterfrom
mmcote:ComponentNamesDevProd
Apr 4, 2018
Merged

Switched getExtraProps to use findClosestFunction to get the component name.#5851
jasonLaster merged 3 commits into
firefox-devtools:masterfrom
mmcote:ComponentNamesDevProd

Conversation

@mmcote

@mmcote mmcote commented Apr 3, 2018

Copy link
Copy Markdown
Contributor

Summary of Changes

  • Changed getExtraProps to use findClosestFunction, which has now been changed to findClosestofSymbolDeclaration to find the name of the component even when in production code.

Screenshots

Before
screen shot 2018-04-03 at 1 58 12 pm

After
screen shot 2018-04-03 at 2 02 36 pm

@jasonLaster jasonLaster left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good!

Comment thread src/utils/ast.js Outdated

export function findClosestFunction(
functions: FunctionDeclaration[],
function findClosestofSymbolDeclaration(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: findClosestofSymbol is smaller :)

Comment thread src/utils/ast.js Outdated
const { classes } = symbols;
if (!classes) {
return null;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can move this to findClosestofSymbol

@darkwing

darkwing commented Apr 4, 2018

Copy link
Copy Markdown
Contributor

Wow, this looks slick @mmcote! Nice work!

So I'm trying to test this on a React app (debugger.html) and I'm not seeing the t or App panes; any idea what I could be doing wrong?

Thanks!

@mmcote

mmcote commented Apr 4, 2018

Copy link
Copy Markdown
Contributor Author

Hey @darkwing, I'm not a hundred percent sure why, and I can look into it, but Jason found a similar issue when debugging the debugger that the react component pane wasn't being triggered when in a component. He suggested switching to a isReactComponent that looks if the class extends Component, PureComponent, etc. Looking through the code I found isReactComponent that does this in the workers/parser/framework.js, more specifically it does this in the method it calls extendsReactComponent. Switching all the isReactComponent's currently being used (which just search for the property _reactInternalInstance or _reactInternalFiber), to this extendsReactComponent seems to fix the issue that Jason was seeing when debugging the debugger. Hopefully this will do the trick for yours as well.

I will push this once I look into the other calls of the new isReactComponent and if they are needed in this case. As currently the new isReactComponent in workers/parser/framework.js also checks if it importsReact or requiresReact which doesn't seem to return true when testing Jason's case.

@jasonLaster
jasonLaster merged commit 8bf255a into firefox-devtools:master Apr 4, 2018
@jasonLaster

Copy link
Copy Markdown
Contributor

We can look into the debugger case separately... This logic works well and the code reads well.

jasonLaster pushed a commit to jasonLaster/debugger.html that referenced this pull request Apr 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants