Skip to content

Add destructors to pooled classes in ReactChildren#4720

Merged
sophiebits merged 1 commit intofacebook:masterfrom
sophiebits:destructor
Aug 28, 2015
Merged

Add destructors to pooled classes in ReactChildren#4720
sophiebits merged 1 commit intofacebook:masterfrom
sophiebits:destructor

Conversation

@sophiebits
Copy link
Copy Markdown
Collaborator

And make destructors mandatory so we're less likely to forget again.

@sophiebits
Copy link
Copy Markdown
Collaborator Author

Note to self: a couple internal users of this will need to be fixed.

@zpao
Copy link
Copy Markdown
Member

zpao commented Aug 27, 2015

Note to @spicyj: we should add a test that makes sure we expect an error when a destructor doesn't exist

@sophiebits
Copy link
Copy Markdown
Collaborator Author

I mean, I don't care that much!

@sophiebits
Copy link
Copy Markdown
Collaborator Author

I am finding hard to imagine a scenario in which adding a test for this would really help. We already have a test making sure the destructor is called if present; no one will accidentally reintroduce an if (instance.destructor) and if they're adding it back on purpose they'd just as soon delete the test checking that it's required.

@zpao
Copy link
Copy Markdown
Member

zpao commented Aug 28, 2015

The scenario is that (edit) somebody who isn't one of us comes along, gets an error in their class which doesn't have a destructor, and decides that PooledClass must need a null check, then gets it reviewed by somebody who didn't see this discussion. They ran the tests and nothing failed so assumed it's an ok change to make since it didn't break expectations. At least if they delete a test they have to acknowledge that they are changing expected behavior.

@sophiebits
Copy link
Copy Markdown
Collaborator Author

But who would add a null check? All pooled classes store some sort of data and thus need a destructor so it wouldn't make sense to make it optional again.

But okay, I'll add a test.

@sophiebits
Copy link
Copy Markdown
Collaborator Author

^^

@zpao
Copy link
Copy Markdown
Member

zpao commented Aug 28, 2015

👍

And make destructors mandatory so we're less likely to forget again.
sophiebits added a commit that referenced this pull request Aug 28, 2015
Add destructors to pooled classes in ReactChildren
@sophiebits sophiebits merged commit 024f715 into facebook:master Aug 28, 2015
elas7 added a commit to elas7/react that referenced this pull request Apr 21, 2016
gaearon pushed a commit that referenced this pull request Jun 26, 2016
zpao pushed a commit that referenced this pull request Jul 8, 2016
They are no longer optional since #4720
(cherry picked from commit 752e159)
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.

3 participants