Receive Server-Side-Events within the UX and display them raw on the Publishing Process view

[sagerb]

Intent

This PR provides the plumbing within the UX to receive the Server-Side-Events being made available from the Agent and updates the Agent code to provide a API endpoint to expose them. The events are displayed on the publish process view after the user hits the publish button on the configuration view.

Resolves #161.

Type of Change

• Bug Fix
• New Feature
• Breaking Change

Approach

• A new endpoint has been added to the agent: /api/events
• The UX opens a connection to this endpoint on the agent during initialization of the app and closes it before the main app component is unmounted.
• All events received are added to an array hosted by the main component and then passed into the publishing process view as a prop.
• The publishing process component shows the event array as raw output and automatically scrolls the window to keep the last event in view.
• The publishing process component also subscribes to the publish complete event and uses it to enable the back button on its view, once publishing has completed. The button is disabled by default.

Automated Tests

• A minor unit test was updated for the agent code.
• e2e tests can now wait on the back button being enabled (data-automation tag of "BackToConfiguration").

Directions for Reviewers

Functionality can be verify by:

• Building the agent and Web UX for this branch: just build-dev
• Starting the agent with valid cmd line parameters. For example:

./bin/darwin-amd64/connect-client publish-ui test/sample-content/fastapi-simple \
--listen=127.0.0.1:9001 \
--open-browser-at="http://127.0.0.1:9000" \
--skip-browser-session-auth \
--account-name=dogfood

• Hit the publish button
• Validate that the server events are displayed
• When they stop and the publish messages indicate the content publishing process was successful, the Back button on the top should be enabled. Clicking it should take you back to the configuration page.

[dotNomad]

Overall this looks fantastic! Really liked that we split off the work for managing these in the store to review just the simplest version of this - which is already pretty complex.

The API for the EventStream is great! I just had a few nitpicks, and bits that I think we can clean up around typing and organization. Also had a few logic questions to be sure I understood everything.

[dotNomad]

Out of curiosity how did you choose between onBeforeUnmount and unmounted?

[sagerb]

When I looked up lifecycle events for guidance, I followed this reference: Destruction Hooks – Cleaning Things Up which said:

Because this is before the component starts to get torn down, this is the time to do most, if not all, of the clean up. At this stage, your component is still fully functional and nothing has been destroyed yet.

It did seem more useful to have some of the component still around just incase I needed it, versus at unmount:

at this point, most of your component and its properties are gone so there’s not much you can do.

[dotNomad]

It is a bit strange to me that this is has size (from the &nbsp) and margin (default from Quasar). I think for this we will want special CSS to keep this from affecting the document flow in the DOM.

[sagerb]

It really is just a no-op html target ref that I needed to be able to scroll down to. This will absolutely change in the next PR and be deleted, although we'll probably need something similar. I was unable to get the visibility scrolling to work unless it actually had a presence within the DOM more than just the node. Totally open to a new way of doing this in the next PR.

[dotNomad]

Really like your import aliasing here and deconstructing to make this a bit easier to read later in the doc.

Nitpicky, but I'd separate the imports from the deconstructing with a whitespace line, but really like your methodology here.

[sagerb]

Thanks, I don't deserve the credit, you'll see that I stole this from one of the Quasar examples.

I also wanted to have a white space between import and const, but the restructuring that I was doing left me wanting to really have an explicit import restructure for those two, so keeping them adjacent was the compromise I came to (with myself). I'll go ahead and add the line, I think I agree with you.

[dotNomad]

This is extremely clean. I really like how simple and understandable the callback here is.

[sagerb]

Thank you!

[dotNomad]

Just want to be sure I'm reading this block correctly.

This is checking that /* is no the first bit of the string, and the subscription type ends with /*? Then from there it checks that the subscription and the event have the same basePath (the bit before /*)?

[sagerb]

Correct. I've added some comments to that section to help us all understand the logic in the future.

[dotNomad]

This does something very similar to the above from the looks of it, but doesn't appear to check that the base is the same. Instead it appears to check if the subscription and incoming event have the same length at the end? That feels incorrect to me, instead shouldn't we check to see if the ending and beginning path match?

This felt a bit tough to read. Perhaps a comment, or splitting these into separate, more bite-sized matching functions wouldn't be helpful. 🤔

[sagerb]

I've added these comments:

    // Are we using a glob, which is meant to be in the middle of two strings
    // which need to be matched
    const globIndex = subscriptionType.indexOf('/**/');
    if (globIndex > 0) {
      // split our subscription type string into two parts (before and after the glob characters)
      const parts = subscriptionType.split('/**/');
      // to match, we must make sure we find that the incoming event type starts
      // exactly with our first part and ends with exactly our second part, regardless of how
      // many characters in the incoming event type are "consumed" by our glob query.
      if (
        incomingEventType.indexOf(parts[0]) === 0 &&
        incomingEventType.indexOf(parts[1]) === incomingEventType.length - parts[1].length
      ) {
        this.logMsg('matched on glob');
        return true;
      }
    }

Does that make more sense?

[dotNomad]

I believe data is optional so we could remove this rather than sending an empty object.

[sagerb]

We have refactored it due to comments above, so now it is required.

[dotNomad]

Suggested change

	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	const rawObj: any = JSON.parse(data);
	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	const obj: any = camelcaseKeys(rawObj);
	// Type safety guard!
	if (isEventStreamMessage(obj)) {
	return obj as EventStreamMessage;
	}
	return null;
	const rawObj = JSON.parse(data);
	const obj = camelcaseKeys(rawObj);
	if (isEventStreamMessage(obj)) {
	return obj;
	}
	return null;

We can actually get rid of all of the any types here and the as. rawObj and obj infer their any types from JSON.parse. isEventStreamMessage also lets us know that obj is a EventSreamMessage so we don't need the as inside the if conditional since obj is now typed as EventStreamMessage

[sagerb]

Great observation, thank you.

[dotNomad]

It is a bit strange to me that this handles both the real case and the mocking case. Perhaps it would be better to separate these and have a parent interface that both implement. That way we can separate the real implementation and the mocking implementation more easily.

[sagerb]

Since we're not yet unit testing this, I'll remove this and we can implement it following your findings with the unit test mocks. I had some ideas for playing around with the concept of production runtime mocking of events, rather than bringing in a different provider, but I'm not so excited about it anymore.