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

Elements from functional components in TestUtils should have no owner #6362

Merged
merged 1 commit into from Apr 14, 2016

Conversation

Projects
None yet
5 participants
@gaearon
Member

gaearon commented Mar 28, 2016

Another take at fixing #5292 and #6194. We’ll fix it more permanently later once we completely remove the owner but it is still currently useful for warnings and ReactPerf.

Reviewers: @sebmarkbage

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 28, 2016

Member

EDIT: nvm. I misread.

cc @spicyj You have a lot more context on this.

Member

sebmarkbage commented Mar 28, 2016

EDIT: nvm. I misread.

cc @spicyj You have a lot more context on this.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 28, 2016

Member

Sure, I’m fine holding it off till 15.0.1 or something along with #6353.
I’ll create a milestone.

Member

gaearon commented Mar 28, 2016

Sure, I’m fine holding it off till 15.0.1 or something along with #6353.
I’ll create a milestone.

@gaearon gaearon added this to the 15.x milestone Mar 28, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 28, 2016

@gaearon updated the pull request.

facebook-github-bot commented Mar 28, 2016

@gaearon updated the pull request.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 28, 2016

Member

I looked but don't know what is best here without spending a lot more time thinking.

Member

sophiebits commented Mar 28, 2016

I looked but don't know what is best here without spending a lot more time thinking.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 28, 2016

Member

What are the bad implications of this? Sure it’s a hack, but it’s pretty much identical to how we fix the same case for render().

Member

gaearon commented Mar 28, 2016

What are the bad implications of this? Sure it’s a hack, but it’s pretty much identical to how we fix the same case for render().

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 31, 2016

Member

I didn't realize we did that for render already. Sure, this seems okay.

Member

sophiebits commented Mar 31, 2016

I didn't realize we did that for render already. Sure, this seems okay.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 31, 2016

Member

Yeah. Owner is a goner anyway so I think we should be able to remove these hacks before 16.

Member

gaearon commented Mar 31, 2016

Yeah. Owner is a goner anyway so I think we should be able to remove these hacks before 16.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 31, 2016

Member

Well, owner is sticking around as long as we support string refs unless I missed something.

Member

sophiebits commented Mar 31, 2016

Well, owner is sticking around as long as we support string refs unless I missed something.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 31, 2016

Member

Can we deprecate them during 15.x?

Member

gaearon commented Mar 31, 2016

Can we deprecate them during 15.x?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Apr 1, 2016

Member

We haven't thought it's worth it because the new API is significantly less convenient.

Member

sophiebits commented Apr 1, 2016

We haven't thought it's worth it because the new API is significantly less convenient.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Apr 1, 2016

Member

We can always consider it, though.

Member

sophiebits commented Apr 1, 2016

We can always consider it, though.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 1, 2016

Member

I generally don't think we should deprecate things unless we're willing or have already done the work internally at FB. (I don't think we are yet. The trade off isn't worth the work.) We use ourselves as a proxy. I don't want to tell others to do work that we're not willing to do ourselves.

Member

sebmarkbage commented Apr 1, 2016

I generally don't think we should deprecate things unless we're willing or have already done the work internally at FB. (I don't think we are yet. The trade off isn't worth the work.) We use ourselves as a proxy. I don't want to tell others to do work that we're not willing to do ourselves.

@gaearon gaearon modified the milestones: 15.x, 15.0.x Apr 14, 2016

@gaearon gaearon merged commit ae56910 into facebook:master Apr 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gaearon gaearon deleted the gaearon:no-owner-in-test-utils branch Apr 14, 2016

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 14, 2016

Member

I’m getting this in since it fixes a bug and the workaround is on par with a similar workaround we’re already using. We have a test too so we won’t regress until this is fixed in a better way.

Member

gaearon commented Apr 14, 2016

I’m getting this in since it fixes a bug and the workaround is on par with a similar workaround we’re already using. We have a test too so we won’t regress until this is fixed in a better way.

@zpao zpao added the semver-patch label Apr 22, 2016

zpao added a commit that referenced this pull request Apr 28, 2016

Merge pull request #6362 from gaearon/no-owner-in-test-utils
Elements from functional components in TestUtils should have no owner
(cherry picked from commit ae56910)

@zpao zpao modified the milestones: 15.0.2, 15.0.x Apr 28, 2016

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v16 #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment