fix(framework): make renderImmediately sync, fix lifecycle issues (#1929

)
SAP · Jul 13, 2020 · 9141300 · 9141300
1 parent d55515f
commit 9141300
Show file tree

Hide file tree

Showing 6 changed files with 186 additions and 82 deletions.
diff --git a/packages/base/src/RenderQueue.js b/packages/base/src/RenderQueue.js
@@ -1,41 +1,65 @@
+const MAX_PROCESS_COUNT = 10;
+
 class RenderQueue {
 	constructor() {
 		this.list = []; // Used to store the web components in order
-		this.promises = new Map(); // Used to store promises for web component rendering
+		this.lookup = new Set(); // Used for faster search
 	}
 
 	add(webComponent) {
-		if (this.promises.has(webComponent)) {
-			return this.promises.get(webComponent);
+		if (this.lookup.has(webComponent)) {
+			return;
 		}
 
-		let deferredResolve;
-		const promise = new Promise(resolve => {
-			deferredResolve = resolve;
-		});
-		promise._deferredResolve = deferredResolve;
-
 		this.list.push(webComponent);
-		this.promises.set(webComponent, promise);
+		this.lookup.add(webComponent);
+	}
+
+	remove(webComponent) {
+		if (!this.lookup.has(webComponent)) {
+			return;
+		}
 
-		return promise;
+		this.list = this.list.filter(item => item !== webComponent);
+		this.lookup.delete(webComponent);
 	}
 
 	shift() {
 		const webComponent = this.list.shift();
 		if (webComponent) {
-			const promise = this.promises.get(webComponent);
-			this.promises.delete(webComponent);
-			return { webComponent, promise };
+			this.lookup.delete(webComponent);
+			return webComponent;
 		}
 	}
 
-	getList() {
-		return this.list;
+	isEmpty() {
+		return this.list.length === 0;
 	}
 
 	isAdded(webComponent) {
-		return this.promises.has(webComponent);
+		return this.lookup.has(webComponent);
+	}
+
+	/**
+	 * Processes the whole queue by executing the callback on each component,
+	 * while also imposing restrictions on how many times a component may be processed.
+	 *
+	 * @param callback - function with one argument (the web component to be processed)
+	 */
+	process(callback) {
+		let webComponent;
+		const stats = new Map();
+
+		webComponent = this.shift();
+		while (webComponent) {
+			const timesProcessed = stats.get(webComponent) || 0;
+			if (timesProcessed > MAX_PROCESS_COUNT) {
+				throw new Error(`Web component processed too many times this task, max allowed is: ${MAX_PROCESS_COUNT}`);
+			}
+			callback(webComponent);
+			stats.set(webComponent, timesProcessed + 1);
+			webComponent = this.shift();
+		}
 	}
 }
 

diff --git a/packages/base/src/RenderScheduler.js b/packages/base/src/RenderScheduler.js
@@ -2,21 +2,18 @@ import RenderQueue from "./RenderQueue.js";
 import { getAllRegisteredTags } from "./CustomElementsRegistry.js";
 import { isRtlAware } from "./locale/RTLAwareRegistry.js";
 
-const MAX_RERENDER_COUNT = 10;
 const registeredElements = new Set();
 
-// Tells whether a render task is currently scheduled
-let renderTaskId;
-
 // Queue for invalidated web components
 const invalidatedWebComponents = new RenderQueue();
 
 let renderTaskPromise,
-	renderTaskPromiseResolve,
-	taskResult;
+	renderTaskPromiseResolve;
 
 let mutationObserverTimer;
 
+let queuePromise;
+
 /**
  * Class that manages the rendering/re-rendering of web components
  * This is always asynchronous
@@ -27,77 +24,65 @@ class RenderScheduler {
 	}
 
 	/**
-	 * Queues a web component for re-rendering
+	 * Schedules a render task (if not already scheduled) to render the component
+	 *
 	 * @param webComponent
+	 * @returns {Promise}
 	 */
-	static renderDeferred(webComponent) {
+	static async renderDeferred(webComponent) {
 		// Enqueue the web component
-		const res = invalidatedWebComponents.add(webComponent);
+		invalidatedWebComponents.add(webComponent);
 
 		// Schedule a rendering task
-		RenderScheduler.scheduleRenderTask();
-		return res;
+		await RenderScheduler.scheduleRenderTask();
 	}
 
+	/**
+	 * Renders a component synchronously
+	 *
+	 * @param webComponent
+	 */
 	static renderImmediately(webComponent) {
-		// Enqueue the web component
-		const res = invalidatedWebComponents.add(webComponent);
-
-		// Immediately start a render task
-		RenderScheduler.runRenderTask();
-		return res;
+		webComponent._render();
 	}
 
 	/**
-	 * Schedules a rendering task, if not scheduled already
+	 * Cancels the rendering of a component, added to the queue with renderDeferred
+	 *
+	 * @param webComponent
 	 */
-	static scheduleRenderTask() {
-		if (!renderTaskId) {
-			// renderTaskId = window.setTimeout(RenderScheduler.renderWebComponents, 3000); // Task
-			// renderTaskId = Promise.resolve().then(RenderScheduler.renderWebComponents); // Micro task
-			renderTaskId = window.requestAnimationFrame(RenderScheduler.renderWebComponents); // AF
-		}
-	}
-
-	static runRenderTask() {
-		if (!renderTaskId) {
-			renderTaskId = 1; // prevent another rendering task from being scheduled, all web components should use this task
-			RenderScheduler.renderWebComponents();
-		}
+	static cancelRender(webComponent) {
+		invalidatedWebComponents.remove(webComponent);
 	}
 
-	static renderWebComponents() {
-		// console.log("------------- NEW RENDER TASK ---------------");
-
-		let webComponentInfo,
-			webComponent,
-			promise;
-		const renderStats = new Map();
-		while (webComponentInfo = invalidatedWebComponents.shift()) { // eslint-disable-line
-			webComponent = webComponentInfo.webComponent;
-			promise = webComponentInfo.promise;
-
-			const timesRerendered = renderStats.get(webComponent) || 0;
-			if (timesRerendered > MAX_RERENDER_COUNT) {
-				// console.warn("WARNING RERENDER", webComponent);
-				throw new Error(`Web component re-rendered too many times this task, max allowed is: ${MAX_RERENDER_COUNT}`);
-			}
-			webComponent._render();
-			promise._deferredResolve();
-			renderStats.set(webComponent, timesRerendered + 1);
-		}
+	/**
+	 * Schedules a rendering task, if not scheduled already
+	 */
+	static async scheduleRenderTask() {
+		if (!queuePromise) {
+			queuePromise = new Promise(resolve => {
+				window.requestAnimationFrame(() => {
+					// Render all components in the queue
+					invalidatedWebComponents.process(component => component._render());
+
+					// Resolve the promise so that callers of renderDeferred can continue
+					queuePromise = null;
+					resolve();
 
-		// wait for Mutation observer just in case
-		if (!mutationObserverTimer) {
-			mutationObserverTimer = setTimeout(() => {
-				mutationObserverTimer = undefined;
-				if (invalidatedWebComponents.getList().length === 0) {
-					RenderScheduler._resolveTaskPromise();
-				}
-			}, 200);
+					// Wait for Mutation observer before the render task is considered finished
+					if (!mutationObserverTimer) {
+						mutationObserverTimer = setTimeout(() => {
+							mutationObserverTimer = undefined;
+							if (invalidatedWebComponents.isEmpty()) {
+								RenderScheduler._resolveTaskPromise();
+							}
+						}, 200);
+					}
+				});
+			});
 		}
 
-		renderTaskId = undefined;
+		await queuePromise;
 	}
 
 	/**
@@ -111,7 +96,7 @@ class RenderScheduler {
 		renderTaskPromise = new Promise(resolve => {
 			renderTaskPromiseResolve = resolve;
 			window.requestAnimationFrame(() => {
-				if (invalidatedWebComponents.getList().length === 0) {
+				if (invalidatedWebComponents.isEmpty()) {
 					renderTaskPromise = undefined;
 					resolve();
 				}
@@ -132,13 +117,13 @@ class RenderScheduler {
 	}
 
 	static _resolveTaskPromise() {
-		if (invalidatedWebComponents.getList().length > 0) {
+		if (!invalidatedWebComponents.isEmpty()) {
 			// More updates are pending. Resolve will be called again
 			return;
 		}
 
 		if (renderTaskPromiseResolve) {
-			renderTaskPromiseResolve.call(this, taskResult);
+			renderTaskPromiseResolve.call(this);
 			renderTaskPromiseResolve = undefined;
 			renderTaskPromise = undefined;
 		}

diff --git a/packages/base/src/UI5Element.js b/packages/base/src/UI5Element.js
@@ -47,6 +47,8 @@ class UI5Element extends HTMLElement {
 		this._upgradeAllProperties();
 		this._initializeContainers();
 		this._upToDate = false;
+		this._inDOM = false;
+		this._fullyConnected = false;
 
 		let deferredResolve;
 		this._domRefReadyPromise = new Promise(resolve => {
@@ -109,6 +111,8 @@ class UI5Element extends HTMLElement {
 		const needsShadowDOM = this.constructor._needsShadowDOM();
 		const slotsAreManaged = this.constructor.getMetadata().slotsAreManaged();
 
+		this._inDOM = true;
+
 		// Render the Shadow DOM
 		if (needsShadowDOM) {
 			if (slotsAreManaged) {
@@ -121,9 +125,14 @@ class UI5Element extends HTMLElement {
 				await Promise.resolve();
 			}
 
+			if (!this._inDOM) { // Component removed from DOM while _processChildren was running
+				return;
+			}
+
 			RenderScheduler.register(this);
 			await RenderScheduler.renderImmediately(this);
 			this._domRefReadyPromise._deferredResolve();
+			this._fullyConnected = true;
 			if (typeof this.onEnterDOM === "function") {
 				this.onEnterDOM();
 			}
@@ -139,20 +148,27 @@ class UI5Element extends HTMLElement {
 		const needsStaticArea = this.constructor._needsStaticArea();
 		const slotsAreManaged = this.constructor.getMetadata().slotsAreManaged();
 
+		this._inDOM = false;
+
 		if (needsShadowDOM) {
 			if (slotsAreManaged) {
 				this._stopObservingDOMChildren();
 			}
 
 			RenderScheduler.deregister(this);
-			if (typeof this.onExitDOM === "function") {
-				this.onExitDOM();
+			if (this._fullyConnected) {
+				if (typeof this.onExitDOM === "function") {
+					this.onExitDOM();
+				}
+				this._fullyConnected = false;
 			}
 		}
 
 		if (needsStaticArea) {
 			this.staticAreaItem._removeFragmentFromStaticArea();
 		}
+
+		RenderScheduler.cancelRender(this);
 	}
 
 	/**

diff --git a/packages/base/test/specs/UI5ElementLifecycle.js b/packages/base/test/specs/UI5ElementLifecycle.js
@@ -79,6 +79,9 @@ describe("Lifecycle works", () => {
 			});
 
 			document.body.appendChild(el);
+
+			await window.RenderScheduler.whenFinished(); // Must wait, otherwise onExitDOM won't be called
+
 			document.body.removeChild(el);
 
 			await window.RenderScheduler.whenFinished();

diff --git a/packages/main/test/pages/DialogLifecycle.html b/packages/main/test/pages/DialogLifecycle.html
@@ -0,0 +1,60 @@
+<!DOCTYPE html>
+<html>
+<head>
+	<meta http-equiv="X-UA-Compatible" content="IE=edge">
+	<meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no">
+	<meta charset="utf-8">
+
+	<title>Button</title>
+
+	<script data-ui5-config type="application/json">
+		{
+			"language": "EN",
+			"noConflict": {
+				"events": ["click"]
+			},
+			"calendarType": "Islamic"
+		}
+	</script>
+
+	<script src="../../webcomponentsjs/webcomponents-loader.js"></script>
+	<script src="../../resources/bundle.esm.js" type="module"></script>
+	<script nomodule src="../../resources/bundle.es5.js"></script>
+
+</head>
+
+<body style="background-color: var(--sapBackgroundColor);">
+
+<ui5-button id="openDialogButton">Open Dialog</ui5-button>
+<br />
+
+<template id="dt">
+	<ui5-dialog id="hello-dialog" header-text="Dialogs are easy!">
+		<div stype="padding: 2rem; display:flex; justify-content: center;">
+			Hello World!
+			<ui5-button id="closeDialogButton">Dismiss</ui5-button>
+		</div>
+	</ui5-dialog>
+</template>
+
+<script>
+	const openBtn = document.getElementById("openDialogButton");
+	openBtn.addEventListener("click", () => {
+		const clone = document.getElementById("dt").content.cloneNode(true);
+		document.body.appendChild(clone);
+		const dialog = document.getElementById("hello-dialog");
+		const closeBtn = dialog.querySelector("#closeDialogButton");
+		closeBtn.addEventListener("click", () => {
+			dialog.addEventListener("afterClose", () => {
+				dialog.parentElement.removeChild(dialog);
+			});
+			dialog.close();
+		});
+
+		document.body.appendChild(dialog);
+		dialog.open();
+	});
+</script>
+
+</body>
+</html>