Merge pull request #2513 from umbraco/v15/feature/grid-inline-editing-fixes

Feature: Block Grid inline editing + Extension Initializers change

Author: nielslyngsoe

src/libs/context-api/consume/context-consumer.controller.ts

@@ -19,7 +19,7 @@ export class UmbContextConsumerController<BaseType = unknown, ResultType extends
 		contextAlias: string | UmbContextToken<BaseType, ResultType>,
 		callback?: UmbContextCallback<ResultType>,
 	) {
-		super(host.getHostElement(), contextAlias, callback);
+		super(() => host.getHostElement(), contextAlias, callback);
 		this.#host = host;
 		host.addUmbController(this);
 	}

src/libs/context-api/consume/context-consumer.test.ts

@@ -7,7 +7,7 @@ import { expect, oneEvent } from '@open-wc/testing';
 
 const testContextAlias = 'my-test-context';
 const testContextAliasAndApiAlias = 'my-test-context#testApi';
-const testContextAliasAndNotExstingApiAlias = 'my-test-context#notExistingTestApi';
+const testContextAliasAndNotExistingApiAlias = 'my-test-context#notExistingTestApi';
 
 class UmbTestContextConsumerClass {
 	public prop: string = 'value from provider';
@@ -74,6 +74,47 @@ describe('UmbContextConsumer', () => {
 			localConsumer.hostConnected();
 		});
 
+		it('works with host as a method', (done) => {
+			const provider = new UmbContextProvider(document.body, testContextAlias, new UmbTestContextConsumerClass());
+			provider.hostConnected();
+
+			const element = document.createElement('div');
+			document.body.appendChild(element);
+
+			const localConsumer = new UmbContextConsumer(
+				() => element,
+				testContextAlias,
+				(_instance: UmbTestContextConsumerClass | undefined) => {
+					if (_instance) {
+						expect(_instance.prop).to.eq('value from provider');
+						done();
+						localConsumer.hostDisconnected();
+						provider.hostDisconnected();
+					}
+				},
+			);
+			localConsumer.hostConnected();
+		});
+
+		it('works with host method returning undefined', async () => {
+			const element = undefined;
+
+			const localConsumer = new UmbContextConsumer(
+				() => element,
+				testContextAlias,
+				(_instance: UmbTestContextConsumerClass | undefined) => {
+					if (_instance) {
+						expect.fail('Callback should not be called when never permitted');
+					}
+				},
+			);
+			localConsumer.hostConnected();
+
+			await Promise.resolve();
+
+			localConsumer.hostDisconnected();
+		});
+
 		/*
 		Unprovided feature is out commented currently. I'm not sure there is a use case. So lets leave the code around until we know for sure.
 		it('acts to Context API disconnected', (done) => {
@@ -139,7 +180,7 @@ describe('UmbContextConsumer', () => {
 			const element = document.createElement('div');
 			document.body.appendChild(element);
 
-			const localConsumer = new UmbContextConsumer(element, testContextAliasAndNotExstingApiAlias, () => {
+			const localConsumer = new UmbContextConsumer(element, testContextAliasAndNotExistingApiAlias, () => {
 				expect(false).to.be.true;
 			});
 			localConsumer.hostConnected();

src/libs/context-api/consume/context-consumer.ts

@@ -3,11 +3,13 @@ import { isUmbContextProvideEventType, UMB_CONTEXT_PROVIDE_EVENT_TYPE } from '..
 import type { UmbContextCallback } from './context-request.event.js';
 import { UmbContextRequestEventImplementation } from './context-request.event.js';
 
+type HostElementMethod = () => Element | undefined;
+
 /**
  * @class UmbContextConsumer
  */
 export class UmbContextConsumer<BaseType = unknown, ResultType extends BaseType = BaseType> {
-	protected _host: Element;
+	protected _retrieveHost: HostElementMethod;
 
 	#skipHost?: boolean;
 	#stopAtContextMatch = true;
@@ -33,11 +35,15 @@ export class UmbContextConsumer<BaseType = unknown, ResultType extends BaseType
 	 * @memberof UmbContextConsumer
 	 */
 	constructor(
-		host: Element,
+		host: Element | HostElementMethod,
 		contextIdentifier: string | UmbContextToken<BaseType, ResultType>,
 		callback?: UmbContextCallback<ResultType>,
 	) {
-		this._host = host;
+		if (typeof host === 'function') {
+			this._retrieveHost = host;
+		} else {
+			this._retrieveHost = () => host;
+		}
 		const idSplit = contextIdentifier.toString().split('#');
 		this.#contextAlias = idSplit[0];
 		this.#apiAlias = idSplit[1] ?? 'default';
@@ -72,6 +78,10 @@ export class UmbContextConsumer<BaseType = unknown, ResultType extends BaseType
 		if (this.#instance === instance) {
 			return true;
 		}
+
+		if (instance === undefined) {
+			throw new Error('Not allowed to set context api instance to undefined.');
+		}
 		if (this.#discriminator) {
 			// Notice if discriminator returns false, we do not want to setInstance.
 			if (this.#discriminator(instance)) {
@@ -126,7 +136,7 @@ export class UmbContextConsumer<BaseType = unknown, ResultType extends BaseType
 			this._onResponse,
 			this.#stopAtContextMatch,
 		);
-		(this.#skipHost ? this._host.parentNode : this._host)?.dispatchEvent(event);
+		(this.#skipHost ? this._retrieveHost()?.parentNode : this._retrieveHost())?.dispatchEvent(event);
 	}
 
 	public hostConnected(): void {
@@ -172,7 +182,7 @@ export class UmbContextConsumer<BaseType = unknown, ResultType extends BaseType
 
 	public destroy(): void {
 		this.hostDisconnected();
-		this._host = undefined as any;
+		this._retrieveHost = undefined as any;
 		this.#callback = undefined;
 		this.#promise = undefined;
 		this.#promiseResolver = undefined;

src/libs/context-api/provide/context-provider.ts

@@ -68,7 +68,7 @@ export class UmbContextProvider<BaseType = unknown, ResultType extends BaseType
 	 * @memberof UmbContextProvider
 	 */
 	public hostConnected(): void {
-		//this.hostElement.addEventListener(UMB_CONTENT_REQUEST_EVENT_TYPE, this.#handleContextRequest);
+		//this.hostElement.addEventListener(UMB_CONTEXT_REQUEST_EVENT_TYPE, this.#handleContextRequest);
 		this.#eventTarget.dispatchEvent(new UmbContextProvideEventImplementation(this.#contextAlias));
 
 		// Listen to our debug event 'umb:debug-contexts'
@@ -79,7 +79,7 @@ export class UmbContextProvider<BaseType = unknown, ResultType extends BaseType
 	 * @memberof UmbContextProvider
 	 */
 	public hostDisconnected(): void {
-		//this.hostElement.removeEventListener(UMB_CONTENT_REQUEST_EVENT_TYPE, this.#handleContextRequest);
+		//this.hostElement.removeEventListener(UMB_CONTEXT_REQUEST_EVENT_TYPE, this.#handleContextRequest);
 		// Out-commented for now, but kept if we like to reintroduce this:
 		//window.dispatchEvent(new UmbContextUnprovidedEventImplementation(this._contextAlias, this.#instance));
 
@@ -103,6 +103,8 @@ export class UmbContextProvider<BaseType = unknown, ResultType extends BaseType
 
 	destroy(): void {
 		this.hostDisconnected();
+		// Note we are not removing the event listener in the hostDisconnected, therefor we do it here [NL].
+		this.#eventTarget?.removeEventListener(UMB_CONTEXT_REQUEST_EVENT_TYPE, this.#handleContextRequest);
 		// We want to call a destroy method on the instance, if it has one.
 		(this.#instance as any)?.destroy?.();
 		this.#instance = undefined;

src/libs/extension-api/controller/base-extension-initializer.controller.test.ts

@@ -59,12 +59,12 @@ class UmbTestConditionAlwaysInvalid extends UmbControllerBase implements UmbExte
 
 describe('UmbBaseExtensionController', () => {
 	describe('Manifest without conditions', () => {
-		//let hostElement: UmbControllerHostElement;
+		let hostElement: UmbControllerHostElement;
 		let extensionRegistry: UmbExtensionRegistry<ManifestWithDynamicConditions>;
 		let manifest: ManifestWithDynamicConditions;
 
 		beforeEach(async () => {
-			//hostElement = await fixture(html`<umb-test-controller-host></umb-test-controller-host>`);
+			hostElement = await fixture(html`<umb-test-controller-host></umb-test-controller-host>`);
 			extensionRegistry = new UmbExtensionRegistry();
 			manifest = {
 				type: 'section',
@@ -74,7 +74,7 @@ describe('UmbBaseExtensionController', () => {
 
 			extensionRegistry.register(manifest);
 		});
-		/*
+
 		it('permits when there is no conditions', (done) => {
 			const extensionController = new UmbTestExtensionController(
 				hostElement,
@@ -92,16 +92,15 @@ describe('UmbBaseExtensionController', () => {
 				},
 			);
 		});
-		*/
 	});
 
 	describe('Manifest with empty conditions', () => {
-		//let hostElement: UmbControllerHostElement;
+		let hostElement: UmbControllerHostElement;
 		let extensionRegistry: UmbExtensionRegistry<ManifestWithDynamicConditions>;
 		let manifest: ManifestWithDynamicConditions;
 
 		beforeEach(async () => {
-			//hostElement = await fixture(html`<umb-test-controller-host></umb-test-controller-host>`);
+			hostElement = await fixture(html`<umb-test-controller-host></umb-test-controller-host>`);
 			extensionRegistry = new UmbExtensionRegistry();
 			manifest = {
 				type: 'section',
@@ -113,7 +112,6 @@ describe('UmbBaseExtensionController', () => {
 			extensionRegistry.register(manifest);
 		});
 
-		/*
 		it('permits when there is empty conditions', (done) => {
 			const extensionController = new UmbTestExtensionController(
 				hostElement,
@@ -124,15 +122,14 @@ describe('UmbBaseExtensionController', () => {
 					if (extensionController.permitted) {
 						expect(extensionController?.manifest?.alias).to.eq('Umb.Test.Section.1');
 
-						// Also verifying that the promise gets resolved.
+						// Also verifying that the promise gets resolved. [NL]
 						extensionController.asPromise().then(() => {
 							done();
 						});
 					}
 				},
 			);
 		});
-		*/
 	});
 
 	describe('Manifest with valid conditions', () => {
@@ -225,14 +222,14 @@ describe('UmbBaseExtensionController', () => {
 					if (isPermitted) {
 						count++;
 						if (count === 1) {
-							// First time render, there is no conditions.
+							// First time render, there is no conditions. [NL]
 							expect(extensionController.manifest?.weight).to.be.equal(2);
 							expect(extensionController.manifest?.conditions?.length).to.be.equal(1);
 						} else if (count === 2) {
-							// Second time render, there is conditions and weight is 22.
+							// Second time render, there is conditions and weight is 22. [NL]
 							expect(extensionController.manifest?.weight).to.be.equal(22);
 							expect(extensionController.manifest?.conditions?.length).to.be.equal(1);
-							// Check that the promise has been resolved for the first render to ensure timing is right.
+							// Check that the promise has been resolved for the first render to ensure timing is right. [NL]
 							expect(initialPromiseResolved).to.be.true;
 							done();
 							extensionController.destroy();
@@ -270,14 +267,14 @@ describe('UmbBaseExtensionController', () => {
 					if (isPermitted) {
 						count++;
 						if (count === 1) {
-							// First time render, there is no conditions.
+							// First time render, there is no conditions. [NL]
 							expect(extensionController.manifest?.weight).to.be.equal(3);
 							expect(extensionController.manifest?.conditions?.length).to.be.equal(0);
 						} else if (count === 2) {
-							// Second time render, there is conditions and weight is 33.
+							// Second time render, there is conditions and weight is 33. [NL]
 							expect(extensionController.manifest?.weight).to.be.equal(33);
 							expect(extensionController.manifest?.conditions?.length).to.be.equal(0);
-							// Check that the promise has been resolved for the first render to ensure timing is right.
+							// Check that the promise has been resolved for the first render to ensure timing is right. [NL]
 							expect(initialPromiseResolved).to.be.true;
 							done();
 							extensionController.destroy();
@@ -315,14 +312,14 @@ describe('UmbBaseExtensionController', () => {
 					if (isPermitted) {
 						count++;
 						if (count === 1) {
-							// First time render, there is no conditions.
+							// First time render, there is no conditions. [NL]
 							expect(extensionController.manifest?.weight).to.be.equal(4);
 							expect(extensionController.manifest?.conditions?.length).to.be.equal(0);
 						} else if (count === 2) {
-							// Second time render, there is conditions and weight is 33.
+							// Second time render, there is conditions and weight is updated. [NL]
 							expect(extensionController.manifest?.weight).to.be.equal(44);
 							expect(extensionController.manifest?.conditions?.length).to.be.equal(1);
-							// Check that the promise has been resolved for the first render to ensure timing is right.
+							// Check that the promise has been resolved for the first render to ensure timing is right. [NL]
 							expect(initialPromiseResolved).to.be.true;
 							done();
 							extensionController.destroy();
@@ -370,14 +367,14 @@ describe('UmbBaseExtensionController', () => {
 					if (isPermitted) {
 						count++;
 						if (count === 1) {
-							// First time render, there is no conditions.
+							// First time render, there is no conditions. [NL]
 							expect(extensionController.manifest?.weight).to.be.undefined;
 							expect(extensionController.manifest?.conditions?.length).to.be.equal(0);
 						} else if (count === 2) {
-							// Second time render, there is a matching kind and then weight is 123.
+							// Second time render, there is a matching kind and then weight is 123. [NL]
 							expect(extensionController.manifest?.weight).to.be.equal(123);
 							expect(extensionController.manifest?.conditions?.length).to.be.equal(0);
-							// Check that the promise has been resolved for the first render to ensure timing is right.
+							// Check that the promise has been resolved for the first render to ensure timing is right. [NL]
 							expect(initialPromiseResolved).to.be.true;
 							done();
 							extensionController.destroy();
@@ -433,7 +430,7 @@ describe('UmbBaseExtensionController', () => {
 				'Umb.Test.Section.1',
 				() => {
 					// This should not be called.
-					expect(true).to.be.false;
+					expect.fail('Callback should not be called when never permitted');
 				},
 			);
 			Promise.resolve().then(() => {
@@ -451,7 +448,7 @@ describe('UmbBaseExtensionController', () => {
 				'Umb.Test.Section.1',
 				() => {
 					// This should not be called.
-					expect(true).to.be.false;
+					expect.fail('Callback should not be called when never permitted');
 				},
 			);
 
@@ -531,7 +528,7 @@ describe('UmbBaseExtensionController', () => {
 				'Umb.Test.Section.1',
 				async () => {
 					count++;
-					// We want the controller callback to first fire when conditions are initialized.
+					// We want the controller callback to first fire when conditions are initialized. [NL]
 					expect(extensionController.manifest?.conditions?.length).to.be.equal(1);
 					expect(extensionController?.manifest?.alias).to.eq('Umb.Test.Section.1');
 					if (count === 1) {
@@ -596,29 +593,30 @@ describe('UmbBaseExtensionController', () => {
 				'Umb.Test.Section.1',
 				async (isPermitted) => {
 					count++;
-					// We want the controller callback to first fire when conditions are initialized.
+					// We want the controller callback to first fire when conditions are initialized. [NL]
 					expect(extensionController.manifest?.conditions?.length).to.be.equal(2);
 					expect(extensionController?.manifest?.alias).to.eq('Umb.Test.Section.1');
 					if (count === 1) {
 						expect(isPermitted).to.be.true;
 						expect(extensionController?.permitted).to.be.true;
-						// Hack to double check that its two conditions that make up the state:
+						// Hack to double check that its two conditions that make up the state: [NL]
 						expect(
 							extensionController.getUmbControllers((controller) => (controller as any).permitted).length,
 						).to.equal(2);
 					} else if (count === 2) {
 						expect(isPermitted).to.be.false;
 						expect(extensionController?.permitted).to.be.false;
-						// Hack to double check that its two conditions that make up the state, in this case its one, cause we already got the callback when one of the conditions changed. meaning in this split second one is still good:
+						// Hack to double check that its two conditions that make up the state, in this case its one, cause we already got the callback when one of the conditions changed. meaning in this split second one is still good: [NL]
 						expect(
 							extensionController.getUmbControllers((controller) => (controller as any).permitted).length,
 						).to.equal(1);
 
 						// Then we are done:
 						extensionController.destroy(); // End this test.
-						setTimeout(() => done(), 60); // Lets wait another round of the conditions approve/disapprove, just to see if the destroy stopped the conditions. (60ms, as that should be enough to test that another round does not happen.)
+						setTimeout(() => done(), 60); // Lets wait another round of the conditions approve/disapprove, just to see if the destroy stopped the conditions. (60ms, as that should be enough to test that another round does not happen.) [NL]
 					} else if (count === 5) {
-						expect(false).to.be.true; // This should not be called.
+						// This should not be called.
+						expect.fail('Callback should not be called when never permitted');
 					}
 				},
 			);