Skip to content

Add support for arrays with holes as placeholders #1440

Merged
marvinhagemeister merged 23 commits into
masterfrom
holes2
Apr 17, 2019
Merged

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

Conversation

@marvinhagemeister

@marvinhagemeister marvinhagemeister commented Mar 22, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

coveralls commented Mar 22, 2019 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

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

This was referenced Mar 24, 2019
Closed
Merged
@developit developit requested review from andrewiggins and developit and removed request for developit March 25, 2019 14:16
andrewiggins
andrewiggins reviewed Mar 25, 2019
View reviewed changes
Comment thread src/diff/children.js Outdated
@Almo7aya

Almo7aya commented Mar 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@andrewiggins

andrewiggins commented Mar 26, 2019

Copy link
Copy Markdown
Member

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

andrewiggins
andrewiggins reviewed Mar 26, 2019
View reviewed changes

@andrewiggins andrewiggins left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is good work! 💯

Comment thread src/diff/index.js Outdated
Comment thread test/browser/focus.test.js Outdated
Comment thread test/browser/toChildArray.test.js Outdated
Comment thread test/browser/components.test.js Outdated
Comment thread test/browser/render.test.js Outdated
Comment thread test/browser/fragments.test.js Outdated
Comment thread src/diff/children.js Outdated
Comment thread src/diff/children.js Outdated
@marvinhagemeister

marvinhagemeister commented Mar 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

@marvinhagemeister marvinhagemeister force-pushed the holes2 branch 2 times, most recently from 29488e8 to a9db477 Compare March 26, 2019 20:22
This was referenced Mar 27, 2019
Closed
Merged
@marvinhagemeister

marvinhagemeister commented Mar 28, 2019

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...

JoviDeCroock
JoviDeCroock approved these changes Apr 2, 2019
View reviewed changes

@JoviDeCroock JoviDeCroock left a comment

Copy link
Copy Markdown
Member

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

marvinhagemeister commented Apr 6, 2019

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!

This was referenced Apr 12, 2019
Closed
Closed
Closed
Closed
marvinhagemeister
marvinhagemeister commented Apr 13, 2019
View reviewed changes
Comment thread src/diff/children.js Outdated
@marvinhagemeister marvinhagemeister mentioned this pull request Apr 14, 2019
Closed
@marvinhagemeister

marvinhagemeister commented Apr 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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 🎉

marvinhagemeister
marvinhagemeister commented Apr 16, 2019
View reviewed changes
Comment thread src/diff/children.js
marvinhagemeister
marvinhagemeister commented Apr 16, 2019
View reviewed changes
Comment thread src/diff/children.js
JoviDeCroock
JoviDeCroock reviewed Apr 17, 2019
View reviewed changes
Comment thread hooks/test/browser/combinations.test.js
JoviDeCroock
JoviDeCroock approved these changes Apr 17, 2019
View reviewed changes

@JoviDeCroock JoviDeCroock left a comment

Copy link
Copy Markdown
Member

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 💯

@marvinhagemeister marvinhagemeister merged commit dd7ef5b into master Apr 17, 2019
@marvinhagemeister marvinhagemeister deleted the holes2 branch April 17, 2019 16:23
This was referenced Apr 21, 2019
Closed
Closed
@JoviDeCroock JoviDeCroock mentioned this pull request Apr 24, 2019
Merged
@marvinhagemeister marvinhagemeister mentioned this pull request Apr 24, 2019
Merged
@mischnic mischnic mentioned this pull request Apr 30, 2019
Closed
@marvinhagemeister marvinhagemeister mentioned this pull request May 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoviDeCroock JoviDeCroock JoviDeCroock approved these changes

@andrewiggins andrewiggins Awaiting requested review from andrewiggins

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

5 participants

@marvinhagemeister @coveralls @Almo7aya @andrewiggins @JoviDeCroock