@samitouri / QOS-React / commits / ef532fd4a4

[Fiber] Fix portal bugs (#8532)

* Enable additional (failing) portal tests * Fix portal unmounting When unmount a portal, we need to unmount *its* children from itself. This is similar to what we would do for a root if we allowed deleting roots. * Skip portals when looking for host siblings A portal is not part of that host tree despite being a child. * Fix comment typo * Add a failing test for portal child reconciliation It is failing because portal bails out of update, seeing null in pendingProps. It is null because we set pendingProps to nextPortal.children, which is null in this test. * Fix the bug when switching to a null portal child If pendingProps is null, we do a bailout in beginWork. This prevents unmounting of the existing child when the new child is null. We fix this by changing portal fiber's pendingProps to be the portal object itself instead of its children. This way, it is never null, and thus doesn't cause a false positive in the bailout condition. * Add a comment about HostPortal in getHostSibling * Revert the fix because I don't know why it worked unmountHostComponents() should have worked despite finding the wrong parent because it should have changed the parent when pushing and popping the portals. * Don't call commitDeletion recursively This leads to a "Cannot find host parent" bug because commitDeletion() clears the return field. When we're inside the loop, we assign node.sibling.return to node.return but by this moment node.return has already been nulled. As a solution we inline code from commitDeletion() without the nulling. It still fails tests but for a different reason (unrelated bug). * Skip portal children in commitNestedUnmounts() We are currently already visiting them in commitUnmount() portal case since it's recursive. This condition avoids visting them twice. * Set node.child.return before going deeper It doesn't seem to influence existing tests but we have this protection in all other similar places. It protects against infinite loops. * Revert "Fix the bug when switching to a null portal child" This reverts commit ed9747deedf11455ba3eb65648007ab99c26ea58. I'll solve this by using an array in place of null instead. * Use [] for empty Portal pendingProps This avoids a false positive bailout with pendingProps == null when portal is empty.

Dan Abramov committed Dec 12, 2016 at 14:18 UTC ef532fd4a4448295ee808c37a9962f031a7086df

5 files changed +110 -15

scripts/fiber/tests-passing.txt

@@ -514,6 +514,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

514 * should render one portal

515 * should render many portals

516 * should render nested portals

517 +* should reconcile portal children

518 * should keep track of namespace across portals (simple)

519 * should keep track of namespace across portals (medium)

520 * should keep track of namespace across portals (complex)

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

+92 -9

@@ -367,6 +367,82 @@ describe('ReactDOMFiber', () => {

367 expect(container.innerHTML).toBe('');

368 });

369

370 + it('should reconcile portal children', () => {

371 + var portalContainer = document.createElement('div');

372 +

373 + ReactDOM.render(

374 + <div>

375 + {ReactDOM.unstable_createPortal(

376 + <div>portal:1</div>,

377 + portalContainer

378 + )}

379 + </div>,

380 + container

381 + );

382 + expect(portalContainer.innerHTML).toBe('<div>portal:1</div>');

383 + expect(container.innerHTML).toBe('<div></div>');

384 +

385 + ReactDOM.render(

386 + <div>

387 + {ReactDOM.unstable_createPortal(

388 + <div>portal:2</div>,

389 + portalContainer

390 + )}

391 + </div>,

392 + container

393 + );

394 + expect(portalContainer.innerHTML).toBe('<div>portal:2</div>');

395 + expect(container.innerHTML).toBe('<div></div>');

396 +

397 + ReactDOM.render(

398 + <div>

399 + {ReactDOM.unstable_createPortal(

400 + <p>portal:3</p>,

401 + portalContainer

402 + )}

403 + </div>,

404 + container

405 + );

406 + expect(portalContainer.innerHTML).toBe('<p>portal:3</p>');

407 + expect(container.innerHTML).toBe('<div></div>');

408 +

409 + ReactDOM.render(

410 + <div>

411 + {ReactDOM.unstable_createPortal(

412 + ['Hi', 'Bye'],

413 + portalContainer

414 + )}

415 + </div>,

416 + container

417 + );

418 + expect(portalContainer.innerHTML).toBe('HiBye');

419 + expect(container.innerHTML).toBe('<div></div>');

420 +

421 + ReactDOM.render(

422 + <div>

423 + {ReactDOM.unstable_createPortal(

424 + ['Bye', 'Hi'],

425 + portalContainer

426 + )}

427 + </div>,

428 + container

429 + );

430 + expect(portalContainer.innerHTML).toBe('ByeHi');

431 + expect(container.innerHTML).toBe('<div></div>');

432 +

433 + ReactDOM.render(

434 + <div>

435 + {ReactDOM.unstable_createPortal(

436 + null,

437 + portalContainer

438 + )}

439 + </div>,

440 + container

441 + );

442 + expect(portalContainer.innerHTML).toBe('');

443 + expect(container.innerHTML).toBe('<div></div>');

444 + });

445 +

446 it('should keep track of namespace across portals (simple)', () => {

447 assertNamespacesMatch(

448 <svg {...expectSVG}>

@@ -400,6 +476,19 @@ describe('ReactDOMFiber', () => {

476 });

477

478 it('should keep track of namespace across portals (medium)', () => {

479 + assertNamespacesMatch(

480 + <svg {...expectSVG}>

481 + <image {...expectSVG} />

482 + {usePortal(

483 + <div {...expectHTML} />

484 + )}

485 + <image {...expectSVG} />

486 + {usePortal(

487 + <div {...expectHTML} />

488 + )}

489 + <image {...expectSVG} />

490 + </svg>

491 + );

492 assertNamespacesMatch(

493 <div {...expectHTML}>

494 <math {...expectMath}>

@@ -613,15 +702,9 @@ describe('ReactDOMFiber', () => {

702 </div>

703 )}

704 </ErrorBoundary>

616 - {

617 - /*

618 - * TODO: enable. Currently this leads to stack overflow

619 - * but it might be a bug in error boundaries rather than SVG or portals.

620 - usePortal(

621 - <div {...expectHTML} />

622 - )

623 - */

624 - }

705 + {usePortal(

706 + <div {...expectHTML} />

707 + )}

708 </svg>

709 );

710 });

src/renderers/shared/fiber/ReactChildFiber.js

+2 -2

@@ -341,7 +341,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {

341 } else {

342 // Update

343 const existing = useFiber(current, priority);

344 - existing.pendingProps = portal.children;

344 + existing.pendingProps = portal.children || [];

345 existing.return = returnFiber;

346 return existing;

347 }

@@ -976,7 +976,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {

976 ) {

977 deleteRemainingChildren(returnFiber, child.sibling);

978 const existing = useFiber(child, priority);

979 - existing.pendingProps = portal.children;

979 + existing.pendingProps = portal.children || [];

980 existing.return = returnFiber;

981 return existing;

982 } else {

src/renderers/shared/fiber/ReactFiber.js

+1 -1

@@ -346,7 +346,7 @@ exports.createFiberFromYield = function(yieldNode : ReactYield, priorityLevel :

346

347 exports.createFiberFromPortal = function(portal : ReactPortal, priorityLevel : PriorityLevel) : Fiber {

348 const fiber = createFiber(HostPortal, portal.key);

349 - fiber.pendingProps = portal.children;

349 + fiber.pendingProps = portal.children || [];

350 fiber.pendingWorkPriority = priorityLevel;

351 fiber.stateNode = {

352 containerInfo: portal.containerInfo,

src/renderers/shared/fiber/ReactFiberCommitWork.js

+14 -3

@@ -152,7 +152,9 @@ module.exports = function<T, P, I, TI, C, CX>(

152 // If we don't have a child, try the siblings instead.

153 continue siblings;

154 }

155 - if (!node.child) {

155 + // If we don't have a child, try the siblings instead.

156 + // We also skip portals because they are not part of this host tree.

157 + if (!node.child || node.tag === HostPortal) {

158 continue siblings;

159 } else {

160 node.child.return = node;

@@ -235,7 +237,9 @@ module.exports = function<T, P, I, TI, C, CX>(

237 let node : Fiber = root;

238 while (true) {

239 commitUnmount(node);

238 - if (node.child) {

240 + // Visit children because they may contain more composite or host nodes.

241 + // Skip portals because commitUnmount() currently visits them recursively.

242 + if (node.child && node.tag !== HostPortal) {

243 // TODO: Coroutines need to visit the stateNode.

244 node.child.return = node;

245 node = node.child;

@@ -265,16 +269,20 @@ module.exports = function<T, P, I, TI, C, CX>(

269 // After all the children have unmounted, it is now safe to remove the

270 // node from the tree.

271 removeChild(parent, node.stateNode);

272 + // Don't visit children because we already visited them.

273 } else if (node.tag === HostPortal) {

274 // When we go into a portal, it becomes the parent to remove from.

275 // We will reassign it back when we pop the portal on the way up.

276 parent = node.stateNode.containerInfo;

277 + // Visit children because portals might contain host components.

278 if (node.child) {

279 + node.child.return = node;

280 node = node.child;

281 continue;

282 }

283 } else {

284 commitUnmount(node);

285 + // Visit children because we may find more host components below.

286 if (node.child) {

287 // TODO: Coroutines need to visit the stateNode.

288 node.child.return = node;

@@ -343,7 +351,10 @@ module.exports = function<T, P, I, TI, C, CX>(

351 }

352 case HostPortal: {

353 // TODO: this is recursive.

346 - commitDeletion(current);

354 + // We are also not using this parent because

355 + // the portal will get pushed immediately.

356 + const parent = getHostParent(current);

357 + unmountHostComponents(parent, current);

358 return;

359 }

360 }