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

Proof of concept. Adding support for array returns from components. #703

Closed
wants to merge 8 commits into from

Conversation

tkh44
Copy link

@tkh44 tkh44 commented May 22, 2017

This is filled with tons of quick hacks and is not meant to be merged. This is a proof of concept and is meant to start the conversation on what is possible and if this is even worth it.

As of opening this PR all karma tests are passing so no breaking changes.

let childComponent = renderedChild && renderedChild.nodeName, toUnmount;

if (typeof childComponent === 'function') {
// set up high order component link
Copy link
Author

Choose a reason for hiding this comment

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

I did not explore this if branch at all.

} else {
toUnmount = inst;

component._component = inst = createComponent(
Copy link
Author

Choose a reason for hiding this comment

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

I think that _component may have to become an array for this to work. createComponent could accept an array and always return an array.

@kossnocorp
Copy link

Yes, please 🙏🏽

@thysultan
Copy link

thysultan commented Aug 16, 2017

From working on this on DIO 8, i've found a few things to take into account in reconciling fragments correctly, for example given the following children...

1, 2, *[3, 4], 5, 6

...assuming the array marked * is the fragment, there is specific behaviour/branches specifically for fragments in regards to.

  • Appending to a fragment.
  • Inserting before a fragment.
  • Inserting the fragment.
  • Appending the fragment.
  • Removing the fragment.
  • and Replacing the fragment(if you use the Node#replaceChild API)

Collectively these make supporting fragments a bit of a pain point without moving to a some variant of a linked list data-structure for children, but never the less worth it considering the opportunities it unlocks.

@leeoniya
Copy link

leeoniya commented Aug 16, 2017

fragments are tough. i had support for them in domvm v2 and dropped them in v3 [1]. you can see from the commit what it takes to get them working right. you must account for:

  • fragments as component roots (the core functionality implemented here)
  • fragments as vnodes (maybe not in preact)
  • arbitrarily nested fragments
  • adjacent fragments
  • nested & adjacent fragments
  • complex fragment reordering/removal while avoiding firing various hooks multiple times.

i had everything but the last bit working, which was ultimately the nail in the coffin.

let's say you have this where every array is a fragment:

[
  a,
  [
    b,
    c,
    [
      d,
      e,
    ],
    f,
  ],
  [
    g,
    h,
    i,
    [
      j,
      k,
    ],
    l,
  ],
  m,
  n,
]

...and your dom looks like this: a,b,c,d,e,f,g,h,i,j,k,l,m,n.

if you have have to swap the entire bc... fragment with the adjacent ghi... fragment, to get the dom to a,g,h,i,j,k,l,b,c,d,e,f,m,n how many ops should this be, and how many lifecycle hooks should be invoked? in domvm, i would have needed to add extra passes prior over the old vtree prior to the final reconciliation to ensure i wasn't firing too many didReinsert events, etc. it's definitely tricky, especially if you have to support wrapper components that also have no dom representation.

another related case is sibling reordering:

[[a,b],[c,d],[e,f],[g,h]]

to

[[c,d],[g,h],[a,b],[e,f]]

you at least double the number of insertBefore ops in the reconciler to get everything in place without having wrapping dom elements. and it gets progressively worse as you add nested fragments.

i may re-add this functionality back, at some point, but i remember writing an exhaustive implementation was quite a pain. perhaps restricting array returns just to component roots may prove easier in Preact, or easier in general due to its different architecture.

at minimum, i think this PR needs to implement and test sibling reordering, nested and adjacent fragment components; rendering a isolated fragment into the dom is a pretty contrived case that sidesteps the really hairy parts.

[1] domvm/domvm@7a111ed#diff-5d2a48e9b3559cd73dac7b0a28bc2d97

@porfirioribeiro
Copy link

Suport returning arrays from components is one of the greatest features added in react 16.
I hope to see it comming to Preact.
Because i want to make my lib compatible with both Preact and React!

@tkh44
Copy link
Author

tkh44 commented Aug 30, 2017

I do not intend to work on this any further due to time constraints and hitting the walls @thysultan and @leeoniya mention. Someone else is more than welcome to take over from here.

@marvinhagemeister
Copy link
Member

Good news! Array returns are supported in Preact X by wrapping any arrays with a Fragment 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants