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

Make _owner from ReactElement non-enumerable #6355

Closed
wants to merge 1 commit into from
Closed

Make _owner from ReactElement non-enumerable #6355

wants to merge 1 commit into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Mar 27, 2016

This makes it invisible to test frameworks which solves #5292 and #6194.

Reviewers: @sebmarkbage

This makes it invisible to test frameworks which solves #5292 and #6194.
var result = shallowRenderer.render(<SomeComponent />);

expect(result.type).toBe('div');
expect(result.props.children).toEqual([
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before this PR, this comparison used to break due to _owner. See https://github.com/bmullan91/react-shallow-renderer-bug for a repro case.

@quantizor
Copy link
Contributor

I believe this will break IE8 compatibility.

@sebmarkbage
Copy link
Collaborator

This is the hottest path in React. Worried about perf. We can do it in DEV but a bit risky if someone uses spread. However I thought this worked at one point. Is it because we didn't attach owner in shallow renderer?

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 27, 2016

I believe this will break IE8 compatibility.

I don’t think so. Still using canDefineProperty and falling back to setting it directly when we can’t. And this is identical to how we already do this with element._source, for example.

However I thought this worked at one point. Is it because we didn't attach owner in shallow renderer?

Asserting on render output for classes works fine as they normally don’t get the _owner in shallow rendering. We have problems in two cases:

The reason these two case are problematic is the following. Normally, for composites, during shallow rendering we replace _renderValidatedComponent with _renderValidatedComponentWithoutOwnerOrContext. This doesn’t seem very nice encapsulation-wise but I guess it does the job.

However, functional components create element during the initial constructor call. Same is true for #5292 where elements end up being created in the constructor. The constructor call is inlined in mountComponent which always sets ReactCurrentOwner, and there is no corresponding _callConstructorWithoutOwnerOrContext. We can introduce it and use a similar pattern as with _renderValidatedComponent if we want to. Would you like to go down that road instead?

@sebmarkbage
Copy link
Collaborator

Setting owner during the constructor was always sketchy. I guess it was for some error message. It should use a different flag. I think I have PR for it somewhere.

Creating elements in the constructor is sketchy too because you might be tempted to mutate it which would break with automatic bailouts for elements.

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 28, 2016

Setting owner during the constructor was always sketchy.

It was added with the first stateless component commit: 5a7bd96. If we remove the owner completely, are we also going to remove information about it from ReactPerf? It’s quite useful as the aggregation key in the inclusive summary.

@sebmarkbage
Copy link
Collaborator

I believe the reason it was added was because in 0.14 we didn't know if something would be a stateless component or not before calling it. With 0.15 we could remove it since we have the isReactComponent check.

We had an idea to infer ownership based on source code location for debugging purposes. The babel plugin that adds source file and line information would let us do that. Not sure if that would be solid enough for ReactPerf.

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 28, 2016

We had an idea to infer ownership based on source code location for debugging purposes. The babel plugin that adds source file and line information would let us do that. Not sure if that would be solid enough for ReactPerf.

Ideally we’d like to know the name of closest enclosing class or createClass() expression but it’s doable with Babel.

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 28, 2016

Superseded by #6362. @sebmarkbage Mind taking a look?

@gaearon gaearon closed this Mar 28, 2016
@gaearon gaearon deleted the hide-owner branch March 28, 2016 20:09
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.

4 participants