Add communication to web views (Web View Controllers and postMessageToWebView)

[tjcouch-sil]

Resolves #1185

• Web View Controllers are network objects create by Web View Providers that represent web views so other extensions can communicate with them,
• WebViewFactory is a new abstract class an extension can implement to combine a Web View Provider with Web View Controllers.
• papi.webViewProviders.postMessageToWebView is a new method for sending messages directly to web views. These are communicated via postMessage and can be received in the web view with window.addEventListener('message', ...)
  • This method is restricted to use only if you have the new webViewNonce which is given to the web view provider via a new third parameter in IWebViewProvider.getWebView
• Added aliases and deprecated some names for clearer method names in the web view service. See Provide interaction with web views on papi via project or scroll group #1185 for more details
• Misc network fixes, logging adjustments, extension restarting fixes, etc.
  • Fixed extension host web socket not properly disconnecting on restart extension host
  • Made disconnecting client log only the method names that are being removed because they were registered to that client instead of listing all method names
• Added sample web view controllers for hello world project web views. The viewer created from a project web view uses the web view controller to communicate with the project web view.
• Added papi.webViews.onDidCloseWebView
• Added useWebViewController hook
• Added globalThis.webViewId on web views

Known Problems:

• useEventAsync has a bug with subscriptions such that it is causing problems with the web view message handler request when moving the web view and causing it to reload (might be the same reason useData's isLoading doesn't work properly)
• Didn't enforce something.something on web view types

---

This change is 

[lyonsil]

Looking great! I'm excited to see this come in.

Here are some initial thoughts. Just passing along what I've reviewed for now to help us get through this sooner. I'll review the rest later today if I get some time.

Reviewed 13 of 27 files at r1, 8 of 12 files at r2, all commit messages.
Reviewable status: 15 of 27 files reviewed, 10 unresolved discussions (waiting on @tjcouch-sil)

---

src/shared/global-this.model.ts line 38 at r1 (raw file):

  var webViewComponent: FunctionComponent<WebViewProps>;
  /** The id of the current web view. Only used in WebView iframes. */
  var webViewId: WebViewId;

My initial reaction to this is "why not just put this in the renderer's global-this?". Actually the same thought applies to all the webview-related items in this file. I guess the Node processes don't really care there is extra stuff, but it seems like we're targeting the wrong scope.

---

src/shared/services/web-view.service-model.ts line 26 at r1 (raw file):

export interface WebViewServiceType {
  /** Event that emits with webView info when a webView is added */
  onDidAddWebView: PlatformEvent<AddWebViewEvent>;

Would this be a good time to change this to onDidOpenWebView to align better with the new onDidCloseWebView? Alternatively, we could change onDidCloseWebView to onDidRemoveWebView to align better with this one. It just seems a little odd to have Add and Close be the matching verbs.

---

src/shared/services/web-view-provider.service.ts line 142 at r1 (raw file):

    webViewProvider,
    WEB_VIEW_PROVIDER_OBJECT_TYPE,
    { webViewType },

I wonder if we're being a little too loose with these attributes. Below we are providing the type and the ID controllers, while here we are only providing the type for providers. Does it make sense to use a single type for attributes of both and make sure we're setting attributes that align with that type?

---

src/shared/services/web-view-provider.service.ts line 182 at r1 (raw file):

 * it unless something else in the existing process is subscribed to it.
 *
 * @param webViewType Type of webView to check for

These comments don't match the function signature. Are we intended to be querying by webViewType or webViewId here?

---

src/shared/services/web-view-provider.service.ts line 239 at r1 (raw file):

  if (webViewControllersById.has(webViewId))
    logger.warn(

Why warn instead of throw?

---

src/shared/services/web-view-provider.service.ts line 284 at r1 (raw file):

export interface WebViewProviderService {
  initialize: typeof initialize;
  hasKnown: typeof hasKnownWebViewProvider;

Perhaps deprecate hasKnown and register and replace them with hasKnownWebViewProvider and registerWebViewProvider? It seems like that would be cleaner/clearer. Same with the services below.

---

src/renderer/components/web-view.component.tsx line 85 at r2 (raw file):

      ([webViewNonce, message, targetOrigin]: Parameters<WebViewMessageRequestHandler>) => {
        if (webViewNonce !== getWebViewNonce(id))
          throw new Error(

Why throw this error but just log the others? I'm not sure why these are treated differently.

---

src/shared/models/web-view-factory.model.ts line 89 at r1 (raw file):

      if (webViewDefinition.id !== webViewId)
        logger.warn(

What not throw instead of warn? It kind of invalidates the point of locking on the original ID, and I'm not sure what else might be impacted.

---

src/renderer/services/web-view.service-host.ts line 534 at r2 (raw file):

 * @param newLayout The changed layout to save.
 */
// TODO: We could filter whether we need to save based on the `direction` argument. - IJH 2023-05-1

It looks like this "TODO" is done. Perhaps more importantly, the params from the docs no longer match the params from the actual function.

---

src/renderer/services/web-view.service-host.ts line 815 at r2 (raw file):

 * for more info.
 */
export function getWebViewNonce(id: WebViewId) {

There is no return type listed for this method.

It might be safer to not export this function but instead export a function that returns whether a provided nonce value matches the nonce for a provided webViewId. Then retrieving nonces can never leave this module, but you still provide a way for other services to check if a nonce they were given is valid.

[tjcouch-sil]

Reviewable status: 8 of 27 files reviewed, 10 unresolved discussions (waiting on @lyonsil)

---

src/renderer/components/web-view.component.tsx line 85 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Why throw this error but just log the others? I'm not sure why these are treated differently.

🤪 good idea. I wrote these at different times and was thinking more about it when I wrote this one haha

---

src/renderer/services/web-view.service-host.ts line 534 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

It looks like this "TODO" is done. Perhaps more importantly, the params from the docs no longer match the params from the actual function.

The TODO is still valid as we are not avoiding saving based on specific directions. I was kinda avoiding writing descriptions for the parameters, but I went ahead and wrote them here anyway.

---

src/renderer/services/web-view.service-host.ts line 815 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

There is no return type listed for this method.

It might be safer to not export this function but instead export a function that returns whether a provided nonce value matches the nonce for a provided webViewId. Then retrieving nonces can never leave this module, but you still provide a way for other services to check if a nonce they were given is valid.

Oh good idea. Duh. Thanks :)

---

src/shared/global-this.model.ts line 38 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

My initial reaction to this is "why not just put this in the renderer's global-this?". Actually the same thought applies to all the webview-related items in this file. I guess the Node processes don't really care there is extra stuff, but it seems like we're targeting the wrong scope.

The global-this files are kinda weird... I believe VS Code does not respect any kind of boundaries in terms of being able to tell where the different globalThis properties are available. I think I put webViewComponent here long ago for some specific reason that I don't remember now, and I just figured I'd put webViewId wherever webViewComponent was since they're pretty similar in some sense. Since they're already a mess without guidance or patterns around what should exactly go where, I'd like to consider the organization of these files another time if that's ok.

---

src/shared/models/web-view-factory.model.ts line 89 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

What not throw instead of warn? It kind of invalidates the point of locking on the original ID, and I'm not sure what else might be impacted.

Eh probably not worth explaining why I did this or leaving it to be honest. Very esoteric and hopefully unused potential functionality of web view providers. I was on the fence anyway. I'll change it. If someone complains, we can always change something about this another time.

---

src/shared/services/web-view-provider.service.ts line 142 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I wonder if we're being a little too loose with these attributes. Below we are providing the type and the ID controllers, while here we are only providing the type for providers. Does it make sense to use a single type for attributes of both and make sure we're setting attributes that align with that type?

I'd think each network object type should be free to have its own consistent type of attributes. Since these are two different object types, I gave them two different types of attributes based on things I thought would be relevant for them. We can discuss more if desired.

---

src/shared/services/web-view-provider.service.ts line 182 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

These comments don't match the function signature. Are we intended to be querying by webViewType or webViewId here?

Oh I copied and pasted this from hasKnownWebViewProvider. Fixed. Thanks for catching.

---

src/shared/services/web-view-provider.service.ts line 239 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Why warn instead of throw?

We already registered the web view controller. We already checked this condition in hasKnownWebViewController, so the only possibility for this happening is if somehow two controllers with the same id got registered at the same exact time and somehow didn't throw an error when they were getting registered on the network. Seems like an exceptionally low chance that this could happen if any at all, and I figured might as well just keep going if we somehow actually hit this point. If we actually hit this line, other bad things have already happened that are much harder to sort out and undo, so let's just note that there was a problem and do our best to keep going so at least the web view controller gets returned and can be properly disposed. Let me know if you still want me to change it to throw an error.

---

src/shared/services/web-view-provider.service.ts line 284 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Perhaps deprecate hasKnown and register and replace them with hasKnownWebViewProvider and registerWebViewProvider? It seems like that would be cleaner/clearer. Same with the services below.

Oh I didn't even process that hasKnown was exposed outside this service. It isn't used anywhere. I'm just going to remove it. Note it isn't on the papi version; just core code.

Good idea on renaming; done.

---

src/shared/services/web-view.service-model.ts line 26 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Would this be a good time to change this to onDidOpenWebView to align better with the new onDidCloseWebView? Alternatively, we could change onDidCloseWebView to onDidRemoveWebView to align better with this one. It just seems a little odd to have Add and Close be the matching verbs.

I thought about this, too, but I just didn't end up taking any action. Done.

I actually started by making it onDidRemoveWebView, but I decided this made no sense and the Add didn't make sense either 😂 so I changed the new one but not the existing one.

[lyonsil]

Reviewed 4 of 9 files at r3, all commit messages.
Reviewable status: 12 of 27 files reviewed, all discussions resolved

---

src/shared/global-this.model.ts line 38 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

The global-this files are kinda weird... I believe VS Code does not respect any kind of boundaries in terms of being able to tell where the different globalThis properties are available. I think I put webViewComponent here long ago for some specific reason that I don't remember now, and I just figured I'd put webViewId wherever webViewComponent was since they're pretty similar in some sense. Since they're already a mess without guidance or patterns around what should exactly go where, I'd like to consider the organization of these files another time if that's ok.

Yeah we can reorganize later. It was just strange to me.

---

src/shared/services/web-view-provider.service.ts line 142 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I'd think each network object type should be free to have its own consistent type of attributes. Since these are two different object types, I gave them two different types of attributes based on things I thought would be relevant for them. We can discuss more if desired.

Ok, makes sense. This is aligned with how we've done this in other places, too, so if we want to formalize types better we can do it in a larger pass.

---

src/shared/services/web-view-provider.service.ts line 239 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

We already registered the web view controller. We already checked this condition in hasKnownWebViewController, so the only possibility for this happening is if somehow two controllers with the same id got registered at the same exact time and somehow didn't throw an error when they were getting registered on the network. Seems like an exceptionally low chance that this could happen if any at all, and I figured might as well just keep going if we somehow actually hit this point. If we actually hit this line, other bad things have already happened that are much harder to sort out and undo, so let's just note that there was a problem and do our best to keep going so at least the web view controller gets returned and can be properly disposed. Let me know if you still want me to change it to throw an error.

I guess in my mind if we got into a really bad spot that's when we'd want to throw instead of limping along. But I don't feel that strongly about it. As you say this shouldn't really happen.

[lyonsil]

Reviewed 8 of 27 files at r1, 2 of 12 files at r2, 5 of 9 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tjcouch-sil)

---

src/declarations/papi-shared-types.ts line 577 at r3 (raw file):

   */
  export interface WebViewControllers {
    'platform.stuffWebView': NetworkableObject<{ doStuff(thing: string): Promise<boolean> }>;

I'm assuming we can erase these two placeholders once we put something real in place, right?

---

src/main/services/extension-host.service.ts line 91 at r3 (raw file):

  // Tells nodemon to restart the process https://github.com/remy/nodemon/blob/HEAD/doc/events.md#using-nodemon-as-child-process
  extensionHost?.send('restart');
  return undefined;

Not a big deal, but I don't think you need a return statement here at all.

---

src/extension-host/services/extension.service.ts line 1212 at r3 (raw file):

      shouldReload = false;
      reloadExtensions(shouldDeactivateExtensions, shouldEmitDidReloadEvent);
    })();

Doesn't this turn into an unawaited promise that we know can throw? Seems like we should at least try/catch around the reloadExtensions call and log the error message.

An alternative approach to this whole thing would be to wrap extension reloading with a mutex so it only runs one at a time. Then any errors would always be seen.

[lyonsil]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

src/extension-host/services/extension.service.ts line 1212 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Doesn't this turn into an unawaited promise that we know can throw? Seems like we should at least try/catch around the reloadExtensions call and log the error message.

An alternative approach to this whole thing would be to wrap extension reloading with a mutex so it only runs one at a time. Then any errors would always be seen.

Thanks!

---

src/main/services/extension-host.service.ts line 91 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Not a big deal, but I don't think you need a return statement here at all.

Thanks!

[lyonsil]

https://tenor.com/view/the-schuyler-sisters-hamilton-schuyler-hamilfilm-angelica-schuyler-gif-17826335

Reviewable status: complete! all files reviewed, all discussions resolved

[tjcouch-sil]

Reviewable status: complete! all files reviewed, all discussions resolved

---

src/declarations/papi-shared-types.ts line 577 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I'm assuming we can erase these two placeholders once we put something real in place, right?

I imagine so :)

---

src/extension-host/services/extension.service.ts line 1212 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Doesn't this turn into an unawaited promise that we know can throw? Seems like we should at least try/catch around the reloadExtensions call and log the error message.

An alternative approach to this whole thing would be to wrap extension reloading with a mutex so it only runs one at a time. Then any errors would always be seen.

Oops! Good find. Added try/catch.

---

src/main/services/extension-host.service.ts line 91 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Not a big deal, but I don't think you need a return statement here at all.

Oh good catch. I guess there was an "inconsistent returns" error at some point, but I guess it went away later.

---

src/shared/services/web-view-provider.service.ts line 239 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I guess in my mind if we got into a really bad spot that's when we'd want to throw instead of limping along. But I don't feel that strongly about it. As you say this shouldn't really happen.

Changed

[irahopkinson]

src/main/services/extension-host.service.ts line 91 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Not a big deal, but I don't think you need a return statement here at all.

Yep, that changed with the type checking PR. We turned off the consistent-return eslint rule and let TSC get it right with noImplicitReturns.

[irahopkinson]

extensions/src/hello-world/src/main.ts line 112 at r2 (raw file):

  // No need to use `this` and implementing an abstract method so can't be static
  // eslint-disable-next-line class-methods-use-this

FYI This seems like it shouldn't be necessary and indeed https://typescript-eslint.io/rules/class-methods-use-this/ accounts for overrides but we are using the eslint rule not this TS one. I'll make a PR to fix-up this rule.