Skip to content

Include rootNode as arg to componentWillUnmount #276

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

Closed
wants to merge 1 commit into from

Conversation

sophiebits
Copy link
Collaborator

Fixes #174.

@yungsters
Copy link
Contributor

I'm fine with this, but I noticed that componentWillUpdate does not have this, either. I'll let @zpao or @jordwalke make the call here.

@sophiebits
Copy link
Collaborator Author

Didn't notice that; adding it to componentWillUpdate would make sense to me for symmetry.

@yungsters
Copy link
Contributor

I agree.

@sophiebits
Copy link
Collaborator Author

Added componentWillUpdate – before we were passing the ReactReconcileTransaction but it wasn't documented anywhere; not sure if anyone in FB relies on that.

As I add all of these I'm becoming less convinced that it's a good idea to pass the root node at all… makes it harder to add arguments in the future if we ever wanted to.

@jordwalke
Copy link
Contributor

I agree with @spicyj here - by passing the DOM node, we touch the DOM when we don't need to. It kind of contradicts the edict of React itself - touch the DOM the minimal amount of times.

The code mod to remove this support might be tough.

@chenglou
Copy link
Contributor

Even though I opened the issue, I'll chime in here: it was kind short-sighted and it's better if the root isn't passed at all. I haven't done any huge components or projects using this yet, so for me and maybe a bunch of other devs out there it's just a matter of adding one line at the beginning of the lifecycle method to rectify this.

@zpao
Copy link
Member

zpao commented Oct 9, 2013

Nothing has happened here and nobody seems excited for what's needs to happen to make this work, so I'm going to make an executive decision and say we won't do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass rootNode for componentWillUnmount
5 participants