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

[Fiber][WIP] Shallow Renderer #8808

Closed
wants to merge 7 commits into from

Conversation

rthor
Copy link
Contributor

@rthor rthor commented Jan 17, 2017

A very early PR, just to keep track of it.

Will need to remove the owner properly, this is
just a proof-of-concept commit. Getting to know
the codebase.
}

getRenderOutput() {
const hasChildren = this.container.children.length > 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The conceptual problem here is composites (e.g. <Foo />) will never end up as children. At least not unless we let the renderer somehow override the resolved element type from function to string conditionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow. This is the "container's" children, which should be the component's root element. Unless I'm mistaken...? This is a very early PR btw. Still wrapping my head around how Fiber works internally :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe too much info/review but:

  • Composite components are the capitalized components like <Foo />
  • Host components are the lowercase ones like <div />

I think what @gaearon is saying is that as a fiber renderer you don’t ever actually get to see the composite components because those are all happened internally so the children you see are the reconciled host component descriptions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this is what I mean.

@koba04
Copy link
Contributor

koba04 commented Jan 24, 2017

Is there any chance ShallowRenderer with Fiber supports all lifecycle methods like #7912?

@gaearon
Copy link
Collaborator

gaearon commented Jan 24, 2017

I would expect that this implementation will be closer to "real" DOM rendering in terms of lifecycle and stuff since it's less hacky.

@koba04
Copy link
Contributor

koba04 commented Jan 24, 2017

That's so nice! I think that makes it easy to add Fiber support into enzyme.

@gaearon
Copy link
Collaborator

gaearon commented Jan 24, 2017

Not sure to be honest. I don't know what's required on Enzyme side but I think they were reaching into internals quite a bit in the past. cc @lelandrichardson @ljharb for thoughts if you have any?

@lelandrichardson
Copy link
Contributor

lelandrichardson commented Jan 24, 2017

@gaearon It's actually not too bad. The only react/lib/* requires we did were for React 0.13, and were to fix a couple of "bugs" (subjective use of the term) that were making it hard to do things in a non-browser environment. All of this is contained here. For React >= 0.14, we don't use any "lib" requires, and a flat file would work fine for us.

The other sin that we commit is to use a lot of internal properties and data structures (ie, internal component instances) that are subject to change in react for traversal and such. If we can align on this that will also be a non-issue.

@gaearon
Copy link
Collaborator

gaearon commented Jan 24, 2017

The other sin that we commit is to use a lot of internal properties and data structures (ie, internal component instances) that are subject to change in react for traversal and such. If we can align on this that will also be a non-issue.

Right, so the problem here is that those are very different in Fiber. Can you point me to the code that traverses the internal structures? Do they absolutely need to be exposed (even via an adapter)? For which use cases?

@lelandrichardson
Copy link
Contributor

@gaearon You can see most of it in this file. If you grep for ._, you will see the access of "private" properties that are accessed.

Do they absolutely need to be exposed (even via an adapter)?

Well, it depends. Most of this could be abstracted away by the react-test-renderer if we extend it to support everything that's needed in the adapter proposal

@gaearon
Copy link
Collaborator

gaearon commented Jan 24, 2017

Do you think somebody from Enzyme could collaborate with us and send the PRs to test renderer to implement what’s missing so that you don’t have to access internal structures? This would make everything so much simpler. I’m happy to provide the necessary guidance.

@lelandrichardson
Copy link
Contributor

@gaearon yes, definitely :)

I've actually already got a branch locally that does this, but I have to update it to reflect my changes to that RFC last night.

@gaearon
Copy link
Collaborator

gaearon commented Jan 24, 2017

Ooh neat. Do keep us updated. Also @iamdustan might be interested in helping out given he’s implemented the Fiber version of RTR.

@lelandrichardson
Copy link
Contributor

@gaearon will do! If everything in that RFC looks reasonable to you all, i'll try and get a PR up ASAP. Probably wouldn't be until thursday, though.

@aweary
Copy link
Contributor

aweary commented Jan 24, 2017

Happy to help with and/or review when ready @lelandrichardson!

@bvaughn
Copy link
Contributor

bvaughn commented Apr 14, 2017

FYI I've started a stab at this over on PR #9426 (see here).

@gaearon
Copy link
Collaborator

gaearon commented May 30, 2017

In the end we went with not using Fiber for that and writing a standalone shallow renderer with no dependencies on other code.

@gaearon gaearon closed this May 30, 2017
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.

9 participants