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.

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

consistent owner for stateless component #6534

Closed
wants to merge 1 commit into from

Conversation

yiminghe
Copy link
Contributor

@yiminghe yiminghe commented Apr 18, 2016

component should not have owner for both create and update lifecycle when in production.(not consistent will cause ref function call)

nextElement._owner !== prevElement._owner ||

@jimfb
Copy link
Contributor

jimfb commented Apr 18, 2016

(not consistent will have a side effect for ref call)

What side effect? How is the code change related to the new unit test? (they look orthogonal at first glance)

@yiminghe
Copy link
Contributor Author

yiminghe commented Apr 19, 2016

you can run the tc. __dev__=true will call ref function once, __dev__=false will call ref function three times

if __dev__ = false

stateless component's element will not have owner when create:

stateless component's element will have owner when update:

ReactCurrentOwner.current = this;

so will trigger ref change:

nextElement._owner !== prevElement._owner ||

@facebook-github-bot
Copy link

@yiminghe updated the pull request.

@yiminghe yiminghe changed the title consistent owner for stateless component consistent owner for component Apr 19, 2016
@yiminghe yiminghe changed the title consistent owner for component consistent stateless owner for component Apr 19, 2016
@yiminghe
Copy link
Contributor Author

yiminghe commented Apr 19, 2016

may related to

#6194

#6362

@yiminghe yiminghe changed the title consistent stateless owner for component consistent owner for stateless component Apr 19, 2016
@facebook-github-bot
Copy link

@yiminghe updated the pull request.

@yiminghe
Copy link
Contributor Author

yiminghe commented Apr 20, 2016

ping @jimfb . better to keep the same behaviour in dev and production environment(except warning message).

@zpao
Copy link
Member

zpao commented Apr 29, 2016

cc @gaearon (seems related to #6362)

@gaearon
Copy link
Collaborator

gaearon commented Apr 29, 2016

Hmm. I don’t think #6362 is related per se.
The code that #6362 removed looked like this already:

-      if (__DEV__) {
-        ReactCurrentOwner.current = this;
-        try {
-          inst = new Component(publicProps, publicContext, ReactUpdateQueue);
-        } finally {
-          ReactCurrentOwner.current = null;
-        }
-      } else {
-        inst = new Component(publicProps, publicContext, ReactUpdateQueue);
-      }

@yiminghe
Copy link
Contributor Author

yiminghe commented May 9, 2016

@gaearon can you run the above tc on current master

@yiminghe
Copy link
Contributor Author

@gaearon @spicyj can you take a look and fix this? One of my library(rc-form) relies ref to keep track of component destruction:

<Select {...controlProps}/>

https://github.com/react-component/form/blob/2b9ca198107011346735141ba39bc9636f5d7c58/src/createBaseForm.jsx#L342

This bug makes our app using stateless function component works fine during development and fails on production.

@ghost
Copy link

ghost commented May 11, 2016

@yiminghe updated the pull request.

@sophiebits
Copy link
Collaborator

I guess this fix is correct. The logic here is very confusing though and I don't know what we can do to avoid bugs in the future. ReactRef should also not check the owner for function refs because there is no reason to, so I amended your commit to check that too and merged as b11540c.

@testones
Copy link

@zpao
Copy link
Member

zpao commented Jul 13, 2016

I missed this since it was closed and pushed separately. I'll get it into the next release.

@zpao zpao modified the milestones: 15-next, 15.3.0 Jul 13, 2016
zpao pushed a commit that referenced this pull request Jul 13, 2016
@germangp088
Copy link

How i can validate a group of checkboxes?

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.

None yet

8 participants