Create host and client functionality for context provider #3

grant-cleary · 2024-03-26T14:41:14Z

PoC of what this host/client stuff could look like. Because the bits that actually hook up to consumers don't exist yet, this is untested and will probably have its share of run-time errors. Naming of things is mostly also provisional at this point.

EDIT: This is no longer a PoC, but a fully-fledged (and tested) implementation. There are still no consumers yet, however.

src/client/client.js

src/host/host.js

src/plugins/plugins.js

dlockhart

Sorry this took so long to get to. This is a quick initial review, I'll circle back and look more closely.

src/client/client.js

src/host/host.js

src/plugins/plugins.js

src/client/client.js

src/host/host.js

dlockhart

This is looking good to me!

src/client/client.js

src/host/host.js

grant-cleary · 2024-04-17T20:00:03Z

This is looking good to me!

Hooray! Okay, so I'm going to implement your suggestions here and get some tests written for these. Once that's done I'll rebase and un-draft/PoC this. 🎉

grant-cleary · 2024-04-23T23:02:59Z

README.md

@@ -10,6 +10,97 @@ Install from CodeArtifact:
 npm install @d2l/lms-context-provider
 ```

+## Usage


I don't love any of this and I can't say I'm great at writing documentation. Very much open to suggestions.

It looks good! One thing I try to personally do when writing docs is to avoid you/your/you're/etc. So like this:

If you do need to initialize a host, simply import and execute the initialize function.

Would become something like this:

To initialize a host, import and execute the initialize() function.

grant-cleary · 2024-04-23T23:04:44Z

test/host.test.js

+		const script = frame.contentWindow.document.createElement('script');
+		script.type = 'text/javascript';
+		script.innerHTML = `window.parent.postMessage(${JSON.stringify(message)}, '*');`;
+		frame.contentWindow.document.head.appendChild(script);


Ah, right, I was meaning to test if I could fake this with an event instead of doing these shenanigans. I'll give that a shot.

It looks like this isn't possible. Any attempt to attach properties to the event (outside of detail) looks to be steadfastly ignored, so I can't mock up the data, source, and origin properties the message handler is expecting. 😞

Cool, worth a shot!

Yeah, I was hoping for a nice workaround here. I'm still a bit baffled that Playwright/Chromium (not sure exactly who's at fault, haha) would disallow injecting a script via srcdoc, but would then be perfectly fine with this hacky business instead. Weird.

grant-cleary · 2024-04-23T23:07:07Z

test/client.test.js

+
+	const setUpIsFramedMessageListener = (frame, spy, respond) => {
+		const handleIsFramedMessage = e => {
+			frame.contentWindow.removeEventListener('message', handleIsFramedMessage);


So this may look a little bit weird... But the easiest way to make sure the client and our mock host are both listening on different windows is to drop an iframe in here and mock out window.parent to hit its contentWindow instead. If we don't do this and we just keep both handlers on the same window, we run into a problem where the client starts intercepting its own requests.

grant-cleary · 2024-04-23T23:08:09Z

src/client/client.js

+
+export async function tryPerform(actionType, options) {
+	await tryPerformImpl(actionType, options);
+}


Wrapping these allows us to restrict what we expose to consumers, while allowing us to fully test the internal implementations.

Would export { tryGetImpl as tryGet, tryPerformImpl as tryPerform } accomplish the same thing, or do they need the wrappers?

Ah, the intent of the wrappers is to allow us to avoid including those internal files in the package's exports at all. It comes at the cost of an extra file to download for anyone implementing this outside of BSI (or something else that uses Rollup), but I kind of like the trade-off. Unless I'm misunderstanding what you mean.

grant-cleary · 2024-04-23T23:10:10Z

package.json

+  "exports": {
+    "./host.js": "./src/host/host.js",
+    "./client.js": "./src/client/client.js"
+  },
  "files": [
    "/src",
    "!demo",


I plan on at least trying to create a demo page for this, even if it means doing similar iframe shenanigans to the test. I don't really want to do that in this PR though because it's not strictly necessary and I'm expecting it to be a boatload of work itself.

That said, these exclusions are kind of redundant since I've structured things such that tests and demos won't live inside the src directory.

So probably don't need this line then?

Yeah, I'll be removing both of these. I have that change hanging out in with the test changes I'm working on.

grant-cleary · 2024-04-23T23:13:36Z

src/host/host-internal.js

+function handleContextRequest(type, options, subscribe) {
+	const plugin = registeredPlugins.get(type);
+
+	if (subscribe && !subscriptionQueue.has(type)) {


This is a change from the PoC. The idea here is that we maintain a single subscription queue for all context types, whether or not they have registered plugins. If we do have a plugin registered, then great, we just call the subscribe callback. If not, and a plugin is registered later, then it will automatically subscribe to anything in this queue.

The end goal here is to be resilient to late plugin registrations. Note that we still return undefined as the immediate response to the tryGet request, as I don't really think it makes sense to do anything else. We can't/shouldn't wait indefinitely.

grant-cleary · 2024-04-23T23:16:55Z

src/host/host-internal.js

+
+	// Process any existing subscription requests
+	if (subscriptionQueue.has(type) && subscriptionCallback) {
+		subscriptionCallback(changedValues => sendChangeEvent(type, changedValues), { sendImmediate: true });


This object with sendImmediate was my thought for how to address this late registration case, since presumably any client listening will want an answer immediately upon registration. This does mean it's up to the host plugin to account for this and immediately execute the callback, but I'm not really sure how else we would go about it. The host can't talk to plugins directly in this callback-based model, so we have no way to directly query the host plugin for data at this point.

I did think about invoking the tryGet here and passing that back, but I think we need to consider the case where the subscription change event may pass back more or different data (or at least in a different format) than the tryGet.

Yeah invoking tryGet here was my initial thought as well -- it's a good point though. My only concern would be that plugin authors would need to handle the possibility of the subscription callback being called before the tryGet, which I initially wouldn't have expected.

Yeah, I'm not 100% sure which is better. On the one hand, if we want to execute the tryGet here, does that imply we're just leaving that client promise unresolved until something answers? Or are we sending back a subscription event with the tryGet value jammed into the changedValues block? I think any of these options are fine as well, but I'm also not sure which would be more expected for a host or client. 🤔

dlockhart

This is looking really good! Mostly nits and a few questions remain.

README.md

dlockhart · 2024-04-25T13:41:24Z

README.md

+allowFrame(myFrame, window.location.origin);
+```
+
+### Using a Client


Using clients will be more common than configuring a host, ya? So might make sense to flip these around in the docs. 🤷

Good point.

README.md

dlockhart · 2024-04-25T13:50:52Z

package.json

+  "exports": {
+    "./host.js": "./src/host/host.js",
+    "./client.js": "./src/client/client.js"
+  },
  "files": [
    "/src",
    "!demo",


So probably don't need this line then?

dlockhart · 2024-04-25T14:32:14Z

src/host/host-internal.js

+
+export function allowFrame(frame, origin) {
+	if (!initialized) {
+		throw new Error(`lms-context-provider: Can't register frame with id ${frame.id}. Context provider host has not been initialized.`);


If you want to get really fancy, you can define your own LmsContextProviderError class that extends Error and it can be the one that pre-pends 'lms-context-provider: ' in front of the messages. Also helpful in your unit tests as you can expect implementations of that class.

Ah, good thought.

dlockhart · 2024-04-25T14:35:33Z

src/host/host-internal.js

+
+	// Process any existing subscription requests
+	if (subscriptionQueue.has(type) && subscriptionCallback) {
+		subscriptionCallback(changedValues => sendChangeEvent(type, changedValues), { sendImmediate: true });


Yeah invoking tryGet here was my initial thought as well -- it's a good point though. My only concern would be that plugin authors would need to handle the possibility of the subscription callback being called before the tryGet, which I initially wouldn't have expected.

dlockhart · 2024-04-25T14:38:03Z

test/client.test.js

+
+describe('lms-context-provider client', () => {
+
+	const sandbox = sinon.createSandbox();


Do you need the sandboxes, or could you just import { restore, spy, stub } from 'sinon' and call that in your afterEach? I haven't found I've needed it unless you need multiple sets of mocks.

Ah, I thought this was required from previously working with pre-Sinon-5 code. But I think the sinon instance is sandboxed itself, so you're right that I don't think this is needed.

dlockhart · 2024-04-25T14:41:10Z

test/client.test.js

+			const listenerSpy = sandbox.spy();
+			setUpIsFramedMessageListener(mockFrame, listenerSpy, false);
+
+			const framed = await isFramed();


This is going to take 75ms to resolve I think? You could try using Sinon's fake timers to move time!

dlockhart · 2024-04-25T14:42:01Z

test/client.test.js

+		frame.contentWindow.addEventListener('message', handleIsFramedMessage);
+	};
+
+	describe('is framed', () => {


Kudos on these tests! They're super easy to follow.

Co-authored-by: Dave Lockhart <[email protected]>

grant-cleary · 2024-04-25T21:32:34Z

test/client.test.js

+				setUpMockHostMessageListener(mockFrame, requestSpy, false);
+
+				const val = tryGet(mockContextType, mockOpts);
+				return val.then(val => {


Not keen on this, but the alternative would be to ditch @brightspace-ui/testing, since from what I can see, chai-as-promised relies on a conflicting major version of chai.

grant-cleary · 2024-04-25T21:34:05Z

src/client/client-internal.js

+const subscriptionCallbacks = new Map();
+
+function handleOneTimeMessageResponse(e) {
+	if (!e?.data?.isContextProvider || !e.data.type) return;


If we don't have this information, we don't actually know which client promise to reject, so I opted to continue to just return nothing here. Throwing might be better, but that still won't reject the promises because this response is async.

grant-cleary · 2024-04-25T21:36:02Z

src/client/client-internal.js

+		document.dispatchEvent(event);
+		return event.detail.handled
+			? event.detail.value
+			: Promise.reject(new LmsContextProviderError('No response from host'));


Technically there are other reasons baked in here... Like if the host rejects the message because it has an invalid origin, etc. But that isn't information available to the client and sending a message back explaining why we weren't sending a message back would be a bit contradictory. 😆

grant-cleary · 2024-04-25T21:41:05Z

src/host/host-internal.js

+function handleContextRequestMessage(e) {
+	if (!e.data.isContextProvider) return;
+	if (!e.data.type || !/^(?:http|https):\/\//.test(e.origin)) {
+		throw new LmsContextProviderError(`Invalid message sent by framed client at origin ${e.origin}`);


There's no action to take beyond this point, because if we've received a message of this sort, we don't want to respond to it. So we throw to log something and make sure the host is aware that something went awry, but we leave it at that.

dlockhart

Looks good to me!

d2l-github-release-tokens · 2024-04-30T17:02:48Z

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

GitHub release
npm package (@latest dist-tag)

Your semantic-release bot 📦🚀