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.

Sign up for GitHub

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

Jump to bottom

Pass shouldHaveDebugID flag to instantiateComponent #7193

Merged
merged 3 commits into from
Jul 5, 2016
Merged

Pass shouldHaveDebugID flag to instantiateComponent #7193

merged 3 commits into from
Jul 5, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jul 5, 2016

This allows us to remove a hack that was added in #6770.
That hack caused #7187 and #7190 so this is an alternative fix to the same problem.

@gaearon gaearon added this to the 15-next milestone Jul 5, 2016
gaearon
gaearon reviewed Jul 5, 2016
View reviewed changes
src/renderers/shared/devtools/__tests__/ReactComponentTreeDevtool-test.js

var Anon = React.createClass({displayName: null, render: () => null});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These changes are just indentation. I moved an existing test into a common describe and added two more tests based on the issue reports.

@gaearon gaearon mentioned this pull request Jul 5, 2016
Closed
@sophiebits
Copy link
Collaborator

Is it possible to just pass in a flag to instantiateReactComponent instead for whether to skip assigning a debug ID? That feels nicer than ad hoc fields.

@gaearon
Pass shouldHaveDebugID flag to instantiateComponent
e24dae0 
This allows us to remove a hack that was added in #6770 and caused #7187 and #7190.
@gaearon gaearon changed the title Detect TopLevelWrapper using a secret field Pass shouldHaveDebugID flag to instantiateComponent Jul 5, 2016
@gaearon
Copy link
Collaborator Author

gaearon commented Jul 5, 2016

Updated.

@sophiebits
Copy link
Collaborator

Can you pass false explicitly the other places it's called? Otherwise good, thanks.

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 5, 2016

I think I did that, did I miss a callsite?

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 5, 2016

More specifically:

  • I pass true in multichild because it always renders “proper” components
  • I pass false in *Mount because it always renders top level wrappers
  • I pass nodeType !== ReactNodeTypes.EMPTY from composites because they may render to null

@sophiebits
Copy link
Collaborator

Oh weird, I think it wasn't showing me all the files. Looks great.

@gaearon gaearon merged commit d2ff462 into facebook:master Jul 5, 2016
@gaearon gaearon mentioned this pull request Jul 5, 2016
Closed
@gaearon gaearon deleted the addenda-bug branch July 7, 2016 18:54
zpao pushed a commit that referenced this pull request Jul 8, 2016
@gaearon @zpao
Pass shouldHaveDebugID flag to instantiateComponent (#7193)
720ce5a 
* Add failing tests for #7187 and #7190

* Pass shouldHaveDebugID flag to instantiateComponent

This allows us to remove a hack that was added in #6770 and caused #7187 and #7190.

* Move logic for choosing whether to use debugID outside instantiate

(cherry picked from commit d2ff462)
@zpao zpao modified the milestones: 15-next, 15.2.1 Jul 8, 2016
usmanajmal pushed a commit to usmanajmal/react that referenced this pull request Jul 11, 2016
@gaearon @usmanajmal
Pass shouldHaveDebugID flag to instantiateComponent (facebook#7193)
00b1ea8 
* Add failing tests for facebook#7187 and facebook#7190

* Pass shouldHaveDebugID flag to instantiateComponent

This allows us to remove a hack that was added in facebook#6770 and caused facebook#7187 and facebook#7190.

* Move logic for choosing whether to use debugID outside instantiate
@renovate renovate bot mentioned this pull request Feb 2, 2018
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sophiebits sophiebits

Labels
CLA Signed
Projects
None yet
Milestone
15.2.1
Development

Successfully merging this pull request may close these issues.
3 participants
@gaearon @sophiebits @zpao