POC - Adding subscription controllers #1901

svanherk · 2021-11-29T20:29:07Z

This create a new ProviderController and SubscriberController that can be used in mixins or components to setup a subscription model. There are pieces I like about it, and pieces I don't. I'll add more context throughout.

svanherk · 2021-11-29T20:32:52Z

components/selection/selection-mixin.js

-		this.removeEventListener('d2l-selection-input-subscribe', this._handleSelectionInputSubscribe);
+	}
+
+	getController(controllerId) {


I don't love this, but I couldn't think of another great way to do both:

Expose the controller without a forced naming convention AND

Have the ability for there to be multiple provider controllers in a single component

github-actions · 2021-11-29T20:39:48Z

Visual diff tests failed - pull request #1902 has been opened with the updated goldens.

svanherk · 2021-11-29T20:39:48Z

helpers/subscriptionControllers.js

+	}
+}
+
+export class SubscriberController {


I don't think this controller reads super well on how you can use either the event method or the for method because the consumer of the controller can set themselves up for both. Tried to add some comments throughout

Would it make sense to just split it into a EventSubscriberController and a ForPropertySubscriberController? Looking at how the logic is split (event fired in hostConnected) and property changes handled in updated, it seems like that could work and be cleaner.

Yeah they're fairly different. The tricky part is that selection components can use either, depending on whether the for property is set, and we don't know which method is being used in the constructor. But attaching both controllers and then just using one of them sounds cleaner, it's probably not a common use case. Filter would just use the ForPropertySubscriberController

Cool I like that.

svanherk · 2021-11-29T20:41:36Z

Still trying to work through the labelledby mixin to see if I can use any of this, but it's really specific. I think I'll open that as a separate PR to determine if it's worth it or not.

dlockhart · 2021-11-30T16:40:17Z

helpers/subscriptionControllers.js

+	}
+}
+
+export class SubscriberController {


Would it make sense to just split it into a EventSubscriberController and a ForPropertySubscriberController? Looking at how the logic is split (event fired in hostConnected) and property changes handled in updated, it seems like that could work and be cleaner.

dlockhart · 2021-11-30T17:41:55Z

helpers/subscriptionControllers.js

+
+	_updateProviders() {
+		let providerIds = this._host[this._forPropertyName];
+		if (typeof(providerIds) === 'string') providerIds = [providerIds];


Shouldn't it always be a string? If we want to support multiple I'd expect it to still be a string but have the IDs be space-separated.

Interesting, I automatically assumed it'd be an array of strings. But string with spaced ids works too

I'm curious what Lit's default attribute processor does for arrays. Like would it turn <d2l-foo ids="one two three"> into an array if that property had an Array type? I'm good with the property being an array as long as that's how the parsing works (which matches what HTML does for things like aria-labelledby).

So looks like no - if I set type to Array and pass in a string, things blow up. It's trying to run JSON.parse(<string_I_pass_in>). It looks like I should be able to write a custom converter to take in the string and convert it to an array. Or we just expect a spaced string and handle it, or just expect an array and handle it.

Yeah I was just thinking about those custom parsers -- if they're easy to write that could be nice.

helpers/subscriptionControllers.js

dlockhart · 2021-11-30T17:49:41Z

helpers/subscriptionControllers.js

@@ -0,0 +1,140 @@
+import { cssEscape } from '../../helpers/dom.js';
+
+export class ProviderController {


For later: I don't love this name since it overlaps with our RequesterMixin/ProviderMixin and could cause some confusion.

Yeah I tried out lots of different names and didn't like most of them. We could do publisher/subscriber, but that's an actual model that this doesn't seem to really follow. Sender/Receiver maybe, but that seems more like a 1-to-1 relationship. Sender/Subscriber?

I don't mind "provider" but maybe just add more context to it? Similar to ForAttributeSubscriberController -- it's a controller for subscribing to the "for" attribute of a host.

SubscriptionProviderController, EventSubscriberController, and IdSubscriberController?

I think those work, ya! Also wondering if we should start a controllers top-level directory... they're similar to mixins in that sense.

dlockhart · 2021-11-30T17:50:54Z

components/selection/selection-observer-mixin.js

-			this.dispatchEvent(evt);
-			this._provider = evt.detail.provider;
-		});
+		this._subscriberController.hostConnected();


This is a good sign -- once we go to Lit 2, connectedCallback, disconnectedCallback and updated can be removed.

svanherk · 2021-11-30T19:44:32Z

helpers/subscriptionControllers.js

+	}
+
+	hostConnected() {
+		if (!this._eventName || this._host[this._forPropertyName]) return;


The weirdest part about splitting this controller into two is this line. Should I add a callback to ask the consumer whether they actually want us to fire the event (or if they ended up getting the for attribute set instead), or do I just send the event? Sending the event when we don't need to means:

It goes off into space and no one actions it, and we don't care

A provider does hear it and adds us as a subscriber, but we basically ignore that provider. There is a hypothetical case where the provider is trying to update us and we don't want it to, but I can specifically protect against that in the selection components

Hmm, I think this highlights that I didn't really understand what was going on here. I assumed that the code was only needed in the EventSubscriberController case, but it looks like additionally if it's using BOTH controllers then it should still not fire the event?

It's only needed for the event case, but the selection components are weird. They basically ask you to provide a selectionFor property, and if you don't, they use the event case. If you do, they use the ForProperty/Attribute case.So I only evere need to use one or the other, but when I'm setting up controllers in the constructor, I don't yet know if we're going to get a for property. I'm assuming I can't setup controllers (the real Lit ones) in the connected callback?

I do think the divided works much better, I'll push that change so you can see

dlockhart · 2021-12-02T16:15:28Z

components/validation/validation-custom-mixin.js

+
+		this._eventSubscriberController = new EventSubscriberController(this, {},
+			{ eventName: 'd2l-validation-custom-connected' }
+		);


Is the event subscriber needed? Seems like the custom validator isn't expecting descendent elements to fire this event.

As far as I can tell, the event is used by FormMixin and FormElementMixin to allow for someone to put a d2l-validation-custom element inside of one of them without adding a for id. Like, if d2l-input-text wanted to instead just put a d2l-validation-custom element inside of itself. There are no examples of this on the demo pages and I had to hack something together to see it happen. When we actually implement this, I think it's worth trying to figure out if anyone actually ever uses this, and decide if that's something we'd still want to support.

Whoa! Yeah I'd definitely lean towards just removing that functionality altogether.

dlockhart · 2021-12-02T16:20:34Z

components/validation/validation-custom-mixin.js

-		return this._forElement;
+		if (this.for) {
+			// Validation custom components only support one for element
+			return this._forPropertySubscriberController.providers[0];


I'd suggest doing an array length check first and return null otherwise. Consumers could have been relying on this being null if no element was connected and now accessing this property will throw an array index out of bounds exception.

dlockhart · 2021-12-02T16:28:08Z

components/validation/validation-custom-mixin.js

-			}
+	_onUnsubscribe() {
+		if (this._forPropertySubscriberController.provider.length === 0) {
+			throw new Error(`validation-custom failed to find element with id ${this.for}`);


When does this end up getting thrown? Seems like before it would throw during connectedCallback -- which arguably could be too soon if there was a delay rendering the element it's looking for.

Having some kind of built-in error throwing capability in the controller itself might be a nice future enhancement too... could simplify the LabelledBy code as well as it does a lot of its own error throwing.

Yeah I didn't test this out before/after but wanted to keep it in to remember - I assume it always "just worked" because people are putting the validator right beside the element. I was looking at adding a "waitTime" to handle LabelledBy anyways, I wonder if I could generalize that to always search for three seconds, and then call an error callback. I don't think we'd setup an error callback for selection or filter because it's not the end of the world if the element doesn't exist, they'll just handle themselves accordingly?

I like the idea of an error callback a lot. 👍

svanherk · 2023-07-11T17:59:39Z

Closing this - #3819 is an up-to-date version.

Add subscription controllers and hookup to the selection components

365512e

svanherk commented Nov 29, 2021

View reviewed changes

github-actions bot mentioned this pull request Nov 29, 2021

Updating Visual Diff Goldens for PR 1901 #1902

Closed

svanherk commented Nov 29, 2021

View reviewed changes

svanherk requested a review from a team November 29, 2021 20:41

dlockhart reviewed Nov 30, 2021

View reviewed changes

svanherk commented Nov 30, 2021

View reviewed changes

svanherk added 2 commits November 30, 2021 16:45

Divide the event and for property subscribers into separate controllers

2307580

Add form and validation POC

015aec7

dlockhart reviewed Dec 2, 2021

View reviewed changes

Add fix and error control (to be tested)

367dda8

svanherk mentioned this pull request Dec 9, 2021

US134384 - Add new subscription controllers #1927

Merged

svanherk mentioned this pull request Jul 11, 2023

POC - Migrate to subscriber controllers #3819

Draft

svanherk closed this Jul 11, 2023

svanherk deleted the POC_Provider_Subscriber_Controllers branch July 11, 2023 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POC - Adding subscription controllers #1901

POC - Adding subscription controllers #1901

svanherk commented Nov 29, 2021

svanherk Nov 29, 2021

github-actions bot commented Nov 29, 2021

svanherk Nov 29, 2021 •

edited

Loading

dlockhart Nov 30, 2021

svanherk Nov 30, 2021

dlockhart Nov 30, 2021

svanherk commented Nov 29, 2021

dlockhart Nov 30, 2021

dlockhart Nov 30, 2021

svanherk Nov 30, 2021

dlockhart Nov 30, 2021

svanherk Nov 30, 2021

dlockhart Dec 1, 2021

dlockhart Nov 30, 2021

svanherk Nov 30, 2021

dlockhart Nov 30, 2021

svanherk Nov 30, 2021

dlockhart Dec 1, 2021

dlockhart Nov 30, 2021

svanherk Nov 30, 2021

dlockhart Nov 30, 2021

svanherk Nov 30, 2021

svanherk Nov 30, 2021

dlockhart Dec 2, 2021

svanherk Dec 2, 2021

dlockhart Dec 2, 2021

dlockhart Dec 2, 2021

dlockhart Dec 2, 2021

svanherk Dec 2, 2021

dlockhart Dec 2, 2021

svanherk commented Jul 11, 2023

		@@ -0,0 +1,140 @@
		import { cssEscape } from '../../helpers/dom.js';

		export class ProviderController {

POC - Adding subscription controllers #1901

POC - Adding subscription controllers #1901

Conversation

svanherk commented Nov 29, 2021

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2021

svanherk Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svanherk commented Nov 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svanherk commented Jul 11, 2023

svanherk Nov 29, 2021 •

edited

Loading