Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add components and classes used by legacy support [URO-190] #176

Merged
merged 13 commits into from
Apr 4, 2024
Merged

Conversation

eapearson
Copy link
Contributor

URO-190 - support for Europa/kbase-ui improvements

This set of changes encompasses support files for Europa/kbase-ui integration improvements. These files are required by those improvements, but are not directly bound to them. That is, they enable new functionality while be decoupled from whatever the legacy support is doing specifically.

I’ve placed the files outside of the src/features/legacy tree into locations which make sense. It is easy enough to move to different locations.

All new and updated files have 100% test coverage.

I’ll describe how each is used:

Alert Component

src/common/components/Alert.tsx

This is a new component which captures the need to display a message with a title and message body as a component an an arbitrary context. It supports two variants, “info” and “error”, which are the only two I’ve needed to support legacy messaging. It can be extended. I didn’t find anything else suitable in the codebase ... there is an existing ErrorPage, but has a bespoke usage as a ErrorBoundary handler.

PageNotFound Component

src/common/components/PageNotFound.tsx

This existing component was updated to add support for an optional message.

It can be useful to impart more information to a user about a missing page or other resource, other than the fact that it is missing.

FallbackNotFound Component

src/common/components/FallbackNotFound.tsx

This new component is used to handle the case in which the “fallback” mechanism is invoked and the requested resource is not one that has a default handler.

When a legacy (kbase-ui) endpoint is invoked and is not supported by kbase-ui, kbase-ui will issue a navigation event to /fallback/X, where X is the originally requested path. If this path is not one that is supported by Europa (and the only one at present is “narratives”), the FallbackNotFound component will be displayed, with the originally requested path provided.

Future changes in legacy support improve path handling, and this helps support one of those cases by allowing us to show the user a somewhat useful error message.

LoadingOverlay Component

src/common/components/LoadingOverlay.tsx

One feature of the improved Europa/kbase-ui integration is control, or at least observation of, the status of kbase-ui loading and initialization.

To facilitate this, and to avoid the “blank iframe” syndrome in which an iframe area is blank (white background) when its content is loading, the LoadingOverlay component is rendered. It is similar to the “cover” used by the Narrative, in which the status of loading the Narrative is displayed until it is fully ready, at which point the “cover” is removed, and the Narrative displayed.

Similarly, the LoadingOverlay covers the iframe’s container, obscuring the underlying content with a loading message and progressive indicator. Upon successful loading of kbase-ui within the iframe, the loading overlay is removed and kbase-ui revealed.

Previously, this would be served by kbase-ui’s own loading overlay. However, this overlay suffers some issues.

First, the messaging implies that the web app is loading, when in reality it (Europa) has already loaded.

Secondly, it is not displayed until kbase-ui itself has started to load. This still allows for a blank iframe. It also does not handle the case of kbase-ui timing out.

Contrary to this design, the loading overlay is controlled by Europa, with messaging updated as kbase-ui progressively loads. This progressive loading is enabled by the upcoming legacy changes, which provides finer grained coordination of loading between kbase-ui and Europa.

TimeoutMonitor Class

lib/common/TimeoutMonitor.ts

This module provides an eponymously named TimeoutMonitor class. This class has the sole responsibility of invoking a callback repeatedly, with a frequency determined by a given interval, and until a given timeout limit is reached. At the timeout limit, another callback is invoked.

The purpose of this class is to allow an independent process to proceed toward some timeout deadline. If some goal is achieved before this timeout, the monitor is canceled. If not, it invokes the provided callback.

It is used in the legacy support to enable a timeout for loading kbase-ui. If the entire kbase-ui loading process is not completed by the given timeout, the iframe is destroyed and an error message displayed.

In addition, as the monitor progresses, it calls the interval callback, which gives us a chance to show a “heartbeat” to indicate that kbase-ui is still loading.

The purpose of this class is to allow monitoring some external process - e.g. a simple value being set, or the state of a component. When the external process meets some condition, the monitor is stopped. If the condition is never met, the timeout is reached, and the timeout callback is invoked.

SendChannel Class

lib/common/SendChannel.ts

The SendChannel class captures the usage of window messages and the postMessage api for sending messages. The partner class “ReceiveChannel” described below is used to receive messages.

The reason to split message transmission into sending and receiving is simply because a common use case, and the base one for Europa, is that communicating with an iframe with a different origin limits window access between the main window and the iframe sub window.

The “channel” concept is one of restricting window communication. Because window communication is open to all javascript code running within the window, some technique is required to ensure that one is receiving only relevant messages. A technique I’ve used successfully with kbase-ui is to assign a unique identifier to a communication session, and only consider messages which include this id. This “channel id” is what establishes a channel. Of course, the message content must also be structured in such a way as to carry both the identifier and the message. Since message “shape” (structure) is another technique to consider only relevant messages, the channel concept can be seen as a way to ensure that even within the context of “the same kind of messages”, only the one’s intended for a specific application are considered.

Another benefit of using a bespoke window message class is that it provides a simpler api for composing messages once the channel object is created.

Another key concept of the channel classes is the separation of message data into a name, envelope, and payload.

The name is used to identify the message and is a simple string. E.g. “kbase-ui.set-title”.

The envelope carries metadata about the message. In this implementation, it carries only the channel id and a unique message instance identifier.

They payload is whatever information the message should convey, in addition to the name itself. In some cases, e.g. a notification, all that is required is the message name, and the payload may be empty, although it is always required. The payload should be an object, as that makes messages relatively self-documenting.

ReceiveChannel

lib/common/ReceiveChannel.ts

A partner to the SendChannel, is the ReceiveChannel. Receiving messages is a more involved task than sending them, as there are more use cases. For instance, the ReceiveClass can handle registering permanent listeners, one-time listeners, buffers messages to cover race conditions, and implements a service model with a start and stop method (since it must register a listener in order to work!)

testUtils

src/testUtils.ts

To facilitate reusable test utilities, the testUtils module was created. At present it supports just two functions to help with testing the window messaging classes SendChannel and ReceiveChannel. These were required because jsDOM postMessage support is incomplete.

These components and classes were extracted from legacy support changes. They do not depend on those changes, although the changes do depend upon them.
@eapearson eapearson requested a review from dakotablair March 19, 2024 16:41
a recent change had converted the onInterval call back to async for a specific use case; but although tests passed, it was broken. This reverts that change, which ends up being unnecessary anyway.
reset message queue before processing, add envelope to listener, add setChannelId method and tests
@eapearson
Copy link
Contributor Author

@dakotablair or @dauglyon wondering if you've had a chance to look at this yet? A few small updates.

@dauglyon
Copy link
Collaborator

Not yet but I should have time this week

@eapearson
Copy link
Contributor Author

@dauglyon thanks, I'm working this morning on adding new tests after a refactoring of Europa/kbase-ui stuff the last couple of days to address some unpleasantness. So that is for the (hopefully) final PR which is built upon the prior PRs. Anyway, my new directive is to keep pushing this so we can get something up in CI in order to be able to demo orcidlink. It doesn't need to be perfect, just usable. So I'm not taking time this week to pave the way for other deployments (we need auth configurations to be repaired, orcidlink service deployed everywhere first) - that will begin again in earnest next week.

Copy link
Collaborator

@dauglyon dauglyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most things are looking good.

I'm not going to be able to really review these files until I see how they're used: RecieveChannel.*, SendChannel.*, TimeoutMonitor.*

As it is, they don't seem to offer much more than the native APIs to me, while also losing some type information, so I'd find myself reaching to those first. Maybe the answer is that these just aren't common/? LMK your thoughts.

Otherwise I've gone through and reviewed the other files, no comments == no concerns ✨

And thanks for all the tests! You're reminding me it's time to get back to our principles about test coverage now that 1.0.7 is released.

src/common/components/Alert.tsx Outdated Show resolved Hide resolved
src/common/components/Alert.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1,33 @@
@import "../../common/colors";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you started to do it here, but we're doing our best to use the use-color function and limit ourselves to the colors in this file, over declaring colors amongst the scss modules

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had actually meant to refactor this, and the comments reminded me.
This has been replaced with OverlayContainer, which ditches the internal loading display in favor of just whatever children are supplied.

src/common/components/Alert.module.scss Outdated Show resolved Hide resolved
src/features/layout/PageNotFound.tsx Show resolved Hide resolved
src/testUtils.ts Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the contents, seems like it could be located somewhere more precise than src/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would normally put it into a "test" folder, but there isn't one. Should I just punted. I suppose "common" would be a good home to get it off of the root.

@dauglyon
Copy link
Collaborator

don't seem to offer much more than the native APIs to me

That said, if we add a required callback for receiveChannel that manages a type guarding / message validation, which either gives us a typed payload or calls onError, I could see how that could be very helpful

something like

export default class ReceiveChannel<PayloadType> {
  /** The window upon which to receive messages */
  window: Window;
  /** Payload type guard and validation, throw or return false to reject the payload. */
  validate: (payload:unknown)=> payload is PayloadType;
 [...]
}
// and
export type ListenerCallback<PayloadType> = (
  payload: PayloadType,
  envelope: MessageEnvelope
) => void;

src/common/SendChannel.ts Outdated Show resolved Hide resolved
There was some reason to abstract this, but I don't remember why, and it isn't used any longer.
The message id is really only useful for debugging, so I suspect it was just an easy way to test with a stable id (rather than relying on the internal detail of the uuid library.)
Anyway, retire this for now, and if the need arises in the future, address it.
The built-in loading display is not being used.
@eapearson
Copy link
Contributor Author

Most things are looking good.

I'm not going to be able to really review these files until I see how they're used: RecieveChannel.*, SendChannel.*, TimeoutMonitor.*

As it is, they don't seem to offer much more than the native APIs to me, while also losing some type information, so I'd find myself reaching to those first. Maybe the answer is that these just aren't common/? LMK your thoughts.

Otherwise I've gone through and reviewed the other files, no comments == no concerns ✨

And thanks for all the tests! You're reminding me it's time to get back to our principles about test coverage now that 1.0.7 is released.

How about I just remove them from here, and include them in the next PR, which uses them? They can then return to the legacy directory, as that is the only place they are used. I removed them from legacy because they are general-purpose, even if not used elsewhere.

@eapearson
Copy link
Contributor Author

That said, if we add a required callback for receiveChannel that manages a type guarding / message validation, which either gives us a typed payload or calls onError, I could see how that could be very helpful

It is a little iess involved, or perhaps better said, a little less coupled. Receivers are free to implement validators for received messages. The api itself ensures the basic structure, the receiver may enforce the validity of the message. In the implementation for legacy support, validation is via message handlers using json schema, but it could be hand-rolled, or could use some other technique or schema. Sometimes it is better to have things a bit less coupled, see what patterns emerge, and possibly later package them up into the api, or often times it is never worth it. Anyway, in this case, I preferred to keep the api simpler and rely upon the usage to add additional functionality like validation and typing.

will be in another PR, in which they are actually used.
@eapearson
Copy link
Contributor Author

Okay, I've removed the SendChannel, ReceiveChannel and TimeoutMonitor, and will include them in the next PR.

BTW I'm not including it, but husky needs additional support to utilize nvm to install/select node when committing from an IDE. I've not included it here, unless you'd like me to, but it is required in order to commit anything (unless one has the proper version of node available globally, in which case there is no point in having nvm configuration in the repo!) (And yes, this is one reason I prefer to work in containers, because then the global environment is guaranteed and consistent for all developers.)

@dauglyon
Copy link
Collaborator

dauglyon commented Apr 2, 2024

Most things are looking good.
I'm not going to be able to really review these files until I see how they're used: RecieveChannel.*, SendChannel.*, TimeoutMonitor.*
As it is, they don't seem to offer much more than the native APIs to me, while also losing some type information, so I'd find myself reaching to those first. Maybe the answer is that these just aren't common/? LMK your thoughts.
Otherwise I've gone through and reviewed the other files, no comments == no concerns ✨
And thanks for all the tests! You're reminding me it's time to get back to our principles about test coverage now that 1.0.7 is released.

How about I just remove them from here, and include them in the next PR, which uses them? They can then return to the legacy directory, as that is the only place they are used. I removed them from legacy because they are general-purpose, even if not used elsewhere.

this sounds good to me for now

BTW I'm not including it, but husky needs additional support to utilize nvm to install/select node when committing from an IDE. I've not included it here, unless you'd like me to, but it is required in order to commit anything (unless one has the proper version of node available globally, in which case there is no point in having nvm configuration in the repo!) (And yes, this is one reason I prefer to work in containers, because then the global environment is guaranteed and consistent for all developers.)

I'm not sure I understand the issue here. You do have NVM installed? But husky isn't running with the right node? I just do a nvm use when I open my IDE. Thart has everything working fine for me. Maybe make a separate PR for this and we can discuss?

Copy link
Collaborator

@dauglyon dauglyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved! See comment above about husky

@eapearson
Copy link
Contributor Author

I'm not sure I understand the issue here. You do have NVM installed? But husky isn't running with the right node? I just do a nvm use when I open my IDE. Thart has everything working fine for me. Maybe make a separate PR for this and we can discuss?

Thanks!
Yes, let us discuss later.
Do you use Visual Studio Code? In VSC, as far as I can tell, husky is run in a separate process, so running nvm use in the ide shell does not affect husky. I mean, if one has node 20 installed as a global default, it will work fine w/o nvm.
I ran across a similar issue in the Narrative, in which husky wants to use Python, but the virtual environment is not available since it runs in a separate process. The virtual environment needs to be initialized in the husky.sh script for it to work.
I attributed that case to some Narrative devs having the required Python dependencies (black, etc.) installed globally, and husky using the default Python without complaint.

@eapearson
Copy link
Contributor Author

@dauglyon thanks, will merge.

@eapearson eapearson merged commit 7e39b6f into main Apr 4, 2024
4 checks passed
@eapearson eapearson deleted the URO-190 branch April 4, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants