Skip to content

Preserve state when using placeholders#2295

Merged
JoviDeCroock merged 3 commits into
masterfrom
feat/improve-double-toChildArray
Feb 1, 2020
Merged

Preserve state when using placeholders#2295
JoviDeCroock merged 3 commits into
masterfrom
feat/improve-double-toChildArray

Conversation

@andrewiggins

@andrewiggins andrewiggins commented Jan 31, 2020

Copy link
Copy Markdown
Member

Add a test and fix a bug where null, false, and true were removed from the render result before going into diffChildren. Calling toChildArray without a callback removes the values mentioned above, which we were doing on the result of render before going into diffChildren.

@github-actions

Copy link
Copy Markdown

Size Change: -6 B (0%)

Total Size: 38.2 kB

Filename Size Change
dist/preact.js 3.69 kB -2 B (0%)
dist/preact.min.js 3.69 kB -2 B (0%)
dist/preact.module.js 3.71 kB -2 B (0%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 2.96 kB 0 B
compat/dist/compat.module.js 2.99 kB 0 B
compat/dist/compat.umd.js 3.01 kB 0 B
debug/dist/debug.js 3.08 kB 0 B
debug/dist/debug.module.js 3.07 kB 0 B
debug/dist/debug.umd.js 3.14 kB 0 B
devtools/dist/devtools.js 175 B 0 B
devtools/dist/devtools.module.js 185 B 0 B
devtools/dist/devtools.umd.js 252 B 0 B
dist/preact.umd.js 3.75 kB 0 B
hooks/dist/hooks.js 1.06 kB 0 B
hooks/dist/hooks.module.js 1.08 kB 0 B
hooks/dist/hooks.umd.js 1.12 kB 0 B
test-utils/dist/testUtils.js 390 B 0 B
test-utils/dist/testUtils.module.js 392 B 0 B
test-utils/dist/testUtils.umd.js 469 B 0 B

compressed-size-action

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 99.808% when pulling 52dbea0 on feat/improve-double-toChildArray into 3834439 on master.

Comment thread src/diff/index.js
newVNode._children = toChildArray(
isTopLevelFragment ? tmp.props.children : tmp
);
newVNode._children = isTopLevelFragment ? tmp.props.children : tmp;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Should this property be renamed to _renderResult since it isn't guaranteed to be an array until after diffChildren is complete? I vaguely remember this confusing people before. I was hesitant to do this immediately cuz it adds yet another property to VNode and this one is particularly short lived (it's just a way to pass the result into diffChildren)

});
});

it('should correctly render when sCU component has null children', () => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This test was in this branch from a while ago. Not sure what it was trying to do but figured it can't hurt 🤷‍♂

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

Nice!

@JoviDeCroock JoviDeCroock merged commit 82e870a into master Feb 1, 2020
@JoviDeCroock JoviDeCroock deleted the feat/improve-double-toChildArray branch February 1, 2020 00:00
porfirioribeiro pushed a commit to porfirioribeiro/preact that referenced this pull request Feb 3, 2020
* Add test with sCU and null children

* Add state-perserving placeholder test

* Remove toChildArray call on render result (-1 B)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants