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

Initialize children if they're null in suspense #2570

Merged
merged 4 commits into from
Jul 13, 2020

Conversation

sventschui
Copy link
Member

@sventschui sventschui commented Jun 3, 2020

Honestly I am currently too tired to fully understand where the race condition happens but this fixes it. If somebody has the power to investigate a bit I'd be more than happy :) Otherwise I'd might look into this in the following days.

@developit you mentioned in one of the issues you have a branch fixing this locally. Could you double check this PR?

Fixes #2519
Fixes #2488

@github-actions
Copy link

github-actions bot commented Jun 3, 2020

Size Change: +16 B (0%)

Total Size: 40 kB

Filename Size Change
compat/dist/compat.js 3.19 kB +6 B (0%)
compat/dist/compat.module.js 3.22 kB +5 B (0%)
compat/dist/compat.umd.js 3.25 kB +5 B (0%)
ℹ️ View Unchanged
Filename Size Change
debug/dist/debug.js 2.99 kB 0 B
debug/dist/debug.module.js 2.98 kB 0 B
debug/dist/debug.umd.js 3.08 kB 0 B
devtools/dist/devtools.js 184 B 0 B
devtools/dist/devtools.module.js 195 B 0 B
devtools/dist/devtools.umd.js 260 B 0 B
dist/preact.js 3.96 kB 0 B
dist/preact.min.js 3.98 kB 0 B
dist/preact.module.js 3.97 kB 0 B
dist/preact.umd.js 4.01 kB 0 B
hooks/dist/hooks.js 1.09 kB 0 B
hooks/dist/hooks.module.js 1.11 kB 0 B
hooks/dist/hooks.umd.js 1.17 kB 0 B
test-utils/dist/testUtils.js 437 B 0 B
test-utils/dist/testUtils.module.js 439 B 0 B
test-utils/dist/testUtils.umd.js 515 B 0 B

compressed-size-action

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

Looked a little more into this, and it is failing because when doing a rerender from App (which is what the setState is doing) and not Suspense, the vnode that is set by diff() on line 170 is a VNode that was created by App's render method. Because it come from App's render method, _children is initialized to null (that vnode hasn't passed through diffChildren yet).

This works whenever the rerender starts with Suspense because when Suspense starts the rerender, we copy all the properties of the oldVNode to the "newVNode", including _children. So when diff() line 170 runs in this case, it happens to assign c._vnode to a vnode that has the old _children property.

Still uncertain what the right fix here is though...


/** @type {() => Promise<void>} */
let resolve;
const Lazy = lazy(() => {
Copy link
Member

Choose a reason for hiding this comment

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

If you want you could use the createLazy helper to simplify this to const [Lazy, resolve] = createLazy() and then pass LazyComp into resolve below

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh okay, just copy&pasted from another test. I'll check

@sventschui
Copy link
Member Author

sventschui commented Jun 4, 2020

Thanks for your comment, paired with a fresh mind it makes things a lot clearer.

I changed the fix to just skip the restore of the _children array when it is null. Makes the test pass too, but still not 100% sure this is the way to go. (But ran out of time in the train so I pushed stuff :) )

EDIT: I guess we should check what the impact of this change is to the re-rendering behaviour of Suspenses‘ children and compare this to react.

@andrewiggins
Copy link
Member

Might be useful to more deeply understand what purpose setting _children[0] serves here and how does it work sometimes to skip it...

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.798% when pulling aff80fe on bugfix/suspense-in-fragment into 4937116 on master.

@JoviDeCroock JoviDeCroock merged commit aea0726 into master Jul 13, 2020
@JoviDeCroock JoviDeCroock deleted the bugfix/suspense-in-fragment branch July 13, 2020 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants