Fix wrong DOM order on suspend inside Fragment by jviide · Pull Request #2107 · preactjs/preact

jviide · 2019-11-10T17:39:26Z

This pull request fixes issue #2106 and refactors the Suspense implementation for smaller code size.

Modify the Suspense lifecycle to be more explicit:
- On the first suspended child set the _detachOnNextRender flag and force a new render with setState.
- On the next render detach the old suspended children from DOM and replace them with an empty list of children.
Use a counter to keep track of the unresolved promises instead of an array of the promises.
Add a test for issue Suspense inside a Fragment may scramble the DOM order #2106.
Fix Suspense test "should correctly render nested Suspense components", it didn't return a promise.
Add a test for children suspending with the same promise.
Add a test ensuring that no children will be rendered if any of them suspends.

The changes in this PR bring down the compat.js.gz bundle size by 38 bytes (from 2700 B to 2662 B).

coveralls · 2019-11-10T17:42:08Z

Coverage decreased (-0.0009%) to 99.891% when pulling a436186 on jviide:suspense-mixup into 3d2edb6 on preactjs:master.

sventschui

As you probably already saw I am super busy the past weeks and didn't spend that much time on preact. I nevertheless wanted to drop some comments here as I find this PR super interesting.

The cave-at with the suspensions/Promise de-duplication should be looked at and I'm super happy to chat here or on slack if you have any questions. Also I try to dig up my de-duplication work and see where this went if I can find some spare minutes.

Anyways thank you for having a look at improving the current implementation :)

andrewiggins · 2019-11-12T01:53:02Z


 /** @jsx createElement */

+async function waitResolved(suspense) {


In another branch I'm rewriting tests that rely on internals. If you are interested, I rewrote these tests to no longer rely on _suspensions if you'd like to pull that change in :)

fa922cb

Sweet! Sorry that I didn't react to this in time. If this PR caused merge conflicts to your branch I can create a patch to resolve them.

jviide · 2019-11-12T16:00:27Z

Updated the implementation. Now DOM is not modified manually at all. Instead the suspended component is removed/detached/lifted from the rendered tree to keep the component's state intact, and added back to the rendered tree when the suspension gets resolved.

The [<Fragment>{props.children?}</Fragment>, props.fallback?] structure that render returns is there because it makes bookkeeping easier: We always know that the (potentially suspended) children are inside the first child of the Suspense component.

This brings the compat.js.gz bundle size further down by 7 bytes (now 2655 B vs. the 2700 B in the master branch, 45 B reduction in total).

marvinhagemeister

So good!! 🙌 ❤️

cristianbote

🎉💪👍 pure magic

marvinhagemeister · 2019-11-12T18:13:45Z

Merging, coveralls is acting up strangely again...

sventschui · 2019-11-12T18:16:12Z

maybe coveralls is right. There are some missed branches in the suspense impl. Might be worth looking at these. I might find some time to do so later today

andrewiggins · 2019-11-12T18:54:28Z

Note, if you click on the "coverage" badge you can see exactly which lines and branches aren't covered

marvinhagemeister · 2019-11-12T22:06:06Z

There were no coverage changes related to any files in this PR. The one were coverage went down was the entry file of preact/debug 🤷‍♂️

prateekbh · 2019-11-12T23:15:16Z

One problem: Anything relying on props.children now gets additional nodes due to the two children.

Anything traversing props.children will probably break. Unless thats an anti pattern for libraries that I am not aware of

jviide · 2019-11-13T02:08:57Z

@prateekbh props.children does not get modified by this code. Could you give an example of a case that you suspect might break?

prateekbh · 2019-11-13T04:31:56Z

You're right I mistakenly added a ghost space in my tests and thought it came because of this

prateekbh · 2019-11-13T04:32:06Z

All good 👍👍👍

Add a test illustrating issue #2106

0a1a261

jviide mentioned this pull request Nov 10, 2019

Suspense inside a Fragment may scramble the DOM order #2106

Closed

jviide added 3 commits November 10, 2019 22:08

Fix test to not use async functions

d7d911b

Fix #2106

c4797f8

Use a counter instead of a list of promises for Suspense bookkeeping

78840d9

jviide changed the title ~~Add a test illustrating issue #2106~~ Fix #2106 Nov 10, 2019

ForsakenHarmony reviewed Nov 10, 2019

View reviewed changes

Comment thread compat/src/suspense.js

jviide added 5 commits November 10, 2019 23:31

Fix tests, remember to return promises

e80f830

Fix Suspense lifecycle

75825e4

Add Suspense._detachOnNextRender to mangle.json

7a91b62

Add _detachOnNextRender to internal.d.ts

fc03587

Make code smaller

434dfd7

marvinhagemeister reviewed Nov 11, 2019

View reviewed changes

Comment thread compat/src/suspense.js Outdated

Comment thread compat/test/browser/suspense.test.js

sventschui reviewed Nov 11, 2019

View reviewed changes

Comment thread compat/src/suspense.js

Comment thread compat/src/suspense.js

Comment thread compat/src/suspense.js Outdated

Comment thread compat/src/suspense.js

jviide added 2 commits November 11, 2019 15:49

Add tests

f0295de

Merge branch 'master' into suspense-mixup

a400fbd

prateekbh reviewed Nov 11, 2019

View reviewed changes

Comment thread compat/src/suspense.js

andrewiggins reviewed Nov 12, 2019

View reviewed changes

jviide added 2 commits November 12, 2019 16:33

Add a new test for suspending component lifecycle calls

994ae6d

Implement suspense without manually modifying DOM

a436186

marvinhagemeister approved these changes Nov 12, 2019

View reviewed changes

cristianbote approved these changes Nov 12, 2019

View reviewed changes

marvinhagemeister changed the title ~~Fix #2106~~ Fix wrong DOM order on suspend inside Fragment Nov 12, 2019

marvinhagemeister merged commit f45b611 into preactjs:master Nov 12, 2019

jviide mentioned this pull request Nov 13, 2019

Fix internal Suspense-related typings #2117

Merged

jviide deleted the suspense-mixup branch November 13, 2019 02:06

snyk-bot mentioned this pull request Mar 8, 2020

[Snyk] Upgrade preact from 10.0.0-beta.3 to 10.3.3 tjenkinson/DefinitelyTyped#8

Open


		/** @jsx createElement */

		async function waitResolved(suspense) {

Uh oh!

Conversation

jviide commented Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sventschui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewiggins Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

jviide Nov 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jviide commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marvinhagemeister left a comment

Choose a reason for hiding this comment

Uh oh!

cristianbote left a comment

Choose a reason for hiding this comment

Uh oh!

marvinhagemeister commented Nov 12, 2019

Uh oh!

sventschui commented Nov 12, 2019

Uh oh!

andrewiggins commented Nov 12, 2019

Uh oh!

marvinhagemeister commented Nov 12, 2019

Uh oh!

prateekbh commented Nov 12, 2019

Uh oh!

jviide commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prateekbh commented Nov 13, 2019

Uh oh!

prateekbh commented Nov 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jviide commented Nov 10, 2019 •

edited

Loading

coveralls commented Nov 10, 2019 •

edited

Loading

jviide commented Nov 12, 2019 •

edited

Loading

jviide commented Nov 13, 2019 •

edited

Loading