Skip to content

Add support for arrays with holes as placeholders #1440

Merged
marvinhagemeister merged 23 commits intomasterfrom
holes2
Apr 17, 2019
Merged

Add support for arrays with holes as placeholders #1440
marvinhagemeister merged 23 commits intomasterfrom
holes2

Conversation

@marvinhagemeister
Copy link
Copy Markdown
Member

@marvinhagemeister marvinhagemeister commented Mar 22, 2019

This PR adds support for tagging empty positions in an array of children. This is best illustrated with the following snippet:

<div>
  {condition && <Foo />} // will be `null` if false
  <div>bar</div>
</div>

The most obvious use case is conditional rendering like in the snippet above. With holes we can effectively skip a lot of work by matching indexes directly.

On top of that the PR includes the following changes:

  • Don't reuse unkeyed components anymore
  • Move focus handling to the top of the tree
    • this change was necessary to make the tests pass. The node that was focussed may be detached from the DOM making it impossible to focus again.

Fixes #1327 , Fixes #753 , Fixes #857 .
Adds +27 B +51 B +44 B +31 B +30 B 🎉

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 22, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 133a4d4 on holes2 into c91f8ac on master.

@developit developit requested review from andrewiggins and developit and removed request for developit March 25, 2019 14:16
@Almo7aya
Copy link
Copy Markdown
Contributor

Almo7aya commented Mar 26, 2019

@marvinhagemeister Are you sure that this PR fixes (#1343)?, I tried your changes, But it still occurred!.

@andrewiggins
Copy link
Copy Markdown
Member

FYI, I have a review pending that I hope to finish today

Copy link
Copy Markdown
Member

@andrewiggins andrewiggins left a comment

Choose a reason for hiding this comment

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

This is good work! 💯

@marvinhagemeister
Copy link
Copy Markdown
Member Author

marvinhagemeister commented Mar 26, 2019

@Almo7aya you're right it doesn't fix it. Removed the mention from the original post.

@marvinhagemeister
Copy link
Copy Markdown
Member Author

I found another way to modify toChildArray so that there are no breaking changes anymore. This makes the changeset a lot smaller. With the recent changes function components will be matched again. The match requirements are now:

  1. Not a class component
  2. Not a functional component with hooks

Or in a nutshell: Don't match anything stateful. This has a size cost though. The PR went up from +27 B to +51 B...

Copy link
Copy Markdown
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

This looks really good, I think this is a good justification for the added bytes

@marvinhagemeister
Copy link
Copy Markdown
Member Author

Waiting on #1515 to be merged first. That one will fix the remaining Fragment bugs and I don't want to get in the way. Stay tuned!

@marvinhagemeister
Copy link
Copy Markdown
Member Author

marvinhagemeister commented Apr 14, 2019

@andrewiggins I've completely reworked my reconciliation strategy. You were right all along that the previous changes were moving into the wrong direction 👍 Would love if you can take another look at this PR 🎉

Copy link
Copy Markdown
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

This looks superb, I can't think of scenario's where implications could arise :D nice work, really seeing this PR grow was awesome 💯

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

Labels

None yet

Projects

None yet

5 participants