Update the webview service to use the new service pattern

[lyonsil]

This change is 

[lyonsil]

The issue said:

When we implement this change for the `web-view.service.ts`, let's move most of the utility code that is in `paranext-dock-layout.components.tsx`

Unfortunately there was some JSX mixed in with the "vanilla" TS code, so refactoring that was going to be difficult without creating strange APIs. If there is still a desire to do that refactoring, we might want to consider a follow up ticket. Alternatively, if someone has an idea of how to do this well then can jump in and add to this PR.

Reviewable status: 0 of 24 files reviewed, all discussions resolved

[tjcouch-sil]

Awesome! This is a sigh of relief haha especially because you can just get the web view service now in getWebView :) thanks!

Reviewed 32 of 32 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyonsil)

---

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

  options: GetWebViewOptions = {},
): Promise<WebViewId | undefined> => {
  // Parameter defaulting doesn't work with network objects

I am concerned that defaulting isn't working here. I believe it should work based on what we are doing with network objects when registering them and creating their proxies and this TS playground test I just made. This seems to be worth further investigation if defaults indeed aren't working.

I am concerned because, if defaulting isn't working here, options is not going to be the correct default value, and getWebViewOptionsDefaults doesn't account for an options that is potentially undefined. Because TS thinks options is defaulted, its possible undefined value sneaks under the radar. I feel like this is a significant issue with the type system if we have to assume any parameter of any network object function could be undefined and not default-able. If defaulting indeed isn't working properly, I wonder if it might be best to remove the defaults in the signature and just default them below every time so it's consistent.

---

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

  getWebView: (
    webViewType: WebViewType,
    layout: Layout | undefined,

I believe layout and options should be optional like layout?: Layout and options?: GetWebViewOptions based on the signature in the service host. Note param: something | undefined is different than param?: something because the question mark makes it optional whereas | undefined means TS will make them put undefined. Since these parameters are defaulted, I believe the question mark is appropriate.

[irahopkinson]

Great job with this. Thanks for taking the time to collect and re-organise logical pieces together in regions.

Reviewed 32 of 32 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lyonsil)

---

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

      window.addEventListener('beforeunload', async () => {
        await unsubscribeRequests();
      });

This section of the code doesn't seem to be moved any where and is just deleted. Do we not need it any more?

Code quote:

      // Register built-in requests
      // TODO: make a registerRequestHandlers function that we use here and in NetworkService.initialize?
      const unsubPromises = Object.entries(rendererRequestHandlers).map(([requestType, handler]) =>
        // Fix type after passing through the `map` function.
        // eslint-disable-next-line no-type-assertion/no-type-assertion
        networkService.registerRequestHandler(requestType as SerializedRequestType, handler),
      );

      // Wait to successfully register all requests
      const unsubscribeRequests = aggregateUnsubscriberAsyncs(await Promise.all(unsubPromises));

      // On closing, try to remove request listeners
      // TODO: should do this on the server when the connection closes or when the server exits as well
      window.addEventListener('beforeunload', async () => {
        await unsubscribeRequests();
      });

---

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

 * HTML or React components.
 */
export interface WebViewServiceType {

I'm not in favor of the recent trend of adding the word Type on to end of type names. Types are indicated in TS by PascalCase. That's why types, interfaces and classes (which are all types) use PascalCase. It also gets confusing when using the word Type in a name legitimately such is the case below on L31 for WebViewType for the type of webViewType. Here and all over the place.

Code quote:

WebViewServiceType

[lyonsil]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

I am concerned that defaulting isn't working here. I believe it should work based on what we are doing with network objects when registering them and creating their proxies and this TS playground test I just made. This seems to be worth further investigation if defaults indeed aren't working.

I am concerned because, if defaulting isn't working here, options is not going to be the correct default value, and getWebViewOptionsDefaults doesn't account for an options that is potentially undefined. Because TS thinks options is defaulted, its possible undefined value sneaks under the radar. I feel like this is a significant issue with the type system if we have to assume any parameter of any network object function could be undefined and not default-able. If defaulting indeed isn't working properly, I wonder if it might be best to remove the defaults in the signature and just default them below every time so it's consistent.

I was getting strange exceptions when I first made the changes. It complained that it couldn't read the type property from null. As I tracked things down, I indeed saw that null was getting through there. I assumed there was something odd like undefinedwas getting turned intonull` on the remote side which was breaking defaulting. Regardless of the low level cause, I had to add this falsy check to get the exception to go away. I made more changes since then, so I'll try to back this out tomorrow and see if the problem still happens.

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

This section of the code doesn't seem to be moved any where and is just deleted. Do we not need it any more?

There was only a single command being registered that I removed. We were registering a command for getWebView, and we were also registering a network object. We didn't need both as they do the same thing, and our normal pattern now is to use network objects over commands, so I removed the command. With it gone, there was no need any longer for the code block you copied here.

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

I'm not in favor of the recent trend of adding the word Type on to end of type names. Types are indicated in TS by PascalCase. That's why types, interfaces and classes (which are all types) use PascalCase. It also gets confusing when using the word Type in a name legitimately such is the case below on L31 for WebViewType for the type of webViewType. Here and all over the place.

Do you have another suggestion here? I think webViewService as the service and WebViewService as the type seems easy to confuse. The same word with one letter of capitalization different makes them kind of easy to confuse. I don't think we have to put Type at the end if there is another idea to make them easy to distinguish.

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

I believe layout and options should be optional like layout?: Layout and options?: GetWebViewOptions based on the signature in the service host. Note param: something | undefined is different than param?: something because the question mark makes it optional whereas | undefined means TS will make them put undefined. Since these parameters are defaulted, I believe the question mark is appropriate.

Ok, I'll make that change. This was actually one of the last things I did because TS started complaining that we were passing null or undefined to this parameter in some of the extension code. I was confused why it didn't complain about that long before I started working on this issue. I wasn't intending to change the function signature.

[lyonsil]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

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

Previously, lyonsil (Matt Lyons) wrote…

I was getting strange exceptions when I first made the changes. It complained that it couldn't read the type property from null. As I tracked things down, I indeed saw that null was getting through there. I assumed there was something odd like undefinedwas getting turned intonull` on the remote side which was breaking defaulting. Regardless of the low level cause, I had to add this falsy check to get the exception to go away. I made more changes since then, so I'll try to back this out tomorrow and see if the problem still happens.

Indeed the error is still there. If I clear out local storage without this if check, I get the following:

[extension host error]
Error: Cannot read properties of null (reading 'type')
at requestUnsafe (/home/lyonsm/paranext-core/src/shared/services/network.service.ts:223:32)
at processTicksAndRejections (node:internal/process/task\_queues:95:5)
Waiting for the debugger to disconnect...
[/extension host error]

If I add it back, the error goes away. I will go ahead and remove the defaults.

[lyonsil]

Reviewable status: 29 of 33 files reviewed, 3 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

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

Previously, lyonsil (Matt Lyons) wrote…

Indeed the error is still there. If I clear out local storage without this if check, I get the following:

[extension host error]
Error: Cannot read properties of null (reading 'type')
at requestUnsafe (/home/lyonsm/paranext-core/src/shared/services/network.service.ts:223:32)
at processTicksAndRejections (node:internal/process/task\_queues:95:5)
Waiting for the debugger to disconnect...
[/extension host error]

If I add it back, the error goes away. I will go ahead and remove the defaults.

Done

---

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

Previously, lyonsil (Matt Lyons) wrote…

Ok, I'll make that change. This was actually one of the last things I did because TS started complaining that we were passing null or undefined to this parameter in some of the extension code. I was confused why it didn't complain about that long before I started working on this issue. I wasn't intending to change the function signature.

Done

[tjcouch-sil]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)

---

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

Previously, lyonsil (Matt Lyons) wrote…

Done

Ah I figured out what's going on with the default values not working anymore. undefined doesn't exist in JSON, so JSON.stringify is turning an undefined in the middle of the array of arguments to null. The reason it was working before is that we were defaulting it on the process before sending it over. It's like if we defaulted the parameters in web-view.service.ts right now (which won't work in general, particularly for extension code).

Good find. I consider this to be a serious issue and flaw in our api to be honest. It's way too common/expected/normal to use defaults for me to feel good about telling people (including reworking all our stuff too) that they can't use them in anything related to commands or network objects in my opinion. I see two ways to move forward (other than accepting our fate) so far:

1. Rework commands and network objects to send objects representing the parameters instead of arrays containing the parameters. Then we can know if keys 0 and 2 are there but not 1, it's an undefined value. Would have to fix it in TS and C# and continue to keep this in mind forever when we're making apis.
2. Add a replacer and reviver function on the JSON.stringify and JSON.parse calls on the lowest level of our network layer on TS and C# that basically replace undefined with some kind of placeholder value like '__papi_undefined or { __papiUndefined: 'yes'} or something that is really weird and unique that is almost guaranteed not to be used anywhere else. Then I guess we could expose utility functions that do these in papi-util or something in case people need them for some other reason. I predict we will already need to do something like this to get classes to serialize and deserialize properly over the websocket at some point. Like the way we do VerseRef right now is so messy for example.

Either way, this will need to be a well-documented requirement for websocket users since this is very not the norm. Seems to me like the benefits of not having to worry about it again (and the simpler implementation) from 2 seem nicer than the slightly more vanilla feel to 1 since we will likely end up having to do something similar to 2 anyway and since doing 2 will make undefined just work in general which seems nice since undefined is meaningfully different than null in a few rare cases (like this one haha).

Thoughts?

I can probably implement this real quick before planning tomorrow. This shouldn't hold up this PR as it is another issue. Feel free to resolve once other comments are resolved and get this in. I just wanted to keep the discussion up for now so we can all see it.

---

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

Previously, lyonsil (Matt Lyons) wrote…

Do you have another suggestion here? I think webViewService as the service and WebViewService as the type seems easy to confuse. The same word with one letter of capitalization different makes them kind of easy to confuse. I don't think we have to put Type at the end if there is another idea to make them easy to distinguish.

I have been thinking the same as Ira. I imagine it's pretty normal to see types for objects as just the capitalized version in TypeScript. Otherwise maybe we could put I on it IWebViewService. Maybe formalize some conditions for when to put I on stuff too

[tjcouch-sil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

Ah I figured out what's going on with the default values not working anymore. undefined doesn't exist in JSON, so JSON.stringify is turning an undefined in the middle of the array of arguments to null. The reason it was working before is that we were defaulting it on the process before sending it over. It's like if we defaulted the parameters in web-view.service.ts right now (which won't work in general, particularly for extension code).

Good find. I consider this to be a serious issue and flaw in our api to be honest. It's way too common/expected/normal to use defaults for me to feel good about telling people (including reworking all our stuff too) that they can't use them in anything related to commands or network objects in my opinion. I see two ways to move forward (other than accepting our fate) so far:

1. Rework commands and network objects to send objects representing the parameters instead of arrays containing the parameters. Then we can know if keys 0 and 2 are there but not 1, it's an undefined value. Would have to fix it in TS and C# and continue to keep this in mind forever when we're making apis.
2. Add a replacer and reviver function on the JSON.stringify and JSON.parse calls on the lowest level of our network layer on TS and C# that basically replace undefined with some kind of placeholder value like '__papi_undefined or { __papiUndefined: 'yes'} or something that is really weird and unique that is almost guaranteed not to be used anywhere else. Then I guess we could expose utility functions that do these in papi-util or something in case people need them for some other reason. I predict we will already need to do something like this to get classes to serialize and deserialize properly over the websocket at some point. Like the way we do VerseRef right now is so messy for example.

Either way, this will need to be a well-documented requirement for websocket users since this is very not the norm. Seems to me like the benefits of not having to worry about it again (and the simpler implementation) from 2 seem nicer than the slightly more vanilla feel to 1 since we will likely end up having to do something similar to 2 anyway and since doing 2 will make undefined just work in general which seems nice since undefined is meaningfully different than null in a few rare cases (like this one haha).

Thoughts?

I can probably implement this real quick before planning tomorrow. This shouldn't hold up this PR as it is another issue. Feel free to resolve once other comments are resolved and get this in. I just wanted to keep the discussion up for now so we can all see it.

Solution 2 could be as simple as something like this playground demonstrates

[lyonsil]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

Solution 2 could be as simple as something like this playground demonstrates

I like 1) more than 2) because, as you say it's vanilla. I think we need to "embrace the boring" more often when the boring works. Also, passing an argument containing all the parameters seems like something that is cleaner to handle when we rework the network protocol. Instead of an array of unnamed objects of unknown types, we can at least have names to reference for each of the parameters. I think if it's an object, we can probably make the serialization/deserialization of network object calls easier to handle in .NET. It also allows us to do some extra verification of arguments and gives us a non-breaking way to add parameters to functions on network objects.

I don't see a need to rush a change in for this right away. I think we can take a little time to breathe and consider.

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

I have been thinking the same as Ira. I imagine it's pretty normal to see types for objects as just the capitalized version in TypeScript. Otherwise maybe we could put I on it IWebViewService. Maybe formalize some conditions for when to put I on stuff too

I generally like to make things obvious, and the difference between "webView" and "WebView" when I'm scanning code is pretty small. I'm happy with an I in front or anything else that makes this stand out. I'm not married to Type at the end by any means. I just don't want anyone working on the code to have to be on their "A game" whenever they open the code because of capitalization.

[tjcouch-sil]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @irahopkinson and @lyonsil)

---

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

Previously, lyonsil (Matt Lyons) wrote…

I like 1) more than 2) because, as you say it's vanilla. I think we need to "embrace the boring" more often when the boring works. Also, passing an argument containing all the parameters seems like something that is cleaner to handle when we rework the network protocol. Instead of an array of unnamed objects of unknown types, we can at least have names to reference for each of the parameters. I think if it's an object, we can probably make the serialization/deserialization of network object calls easier to handle in .NET. It also allows us to do some extra verification of arguments and gives us a non-breaking way to add parameters to functions on network objects.

I don't see a need to rush a change in for this right away. I think we can take a little time to breathe and consider.

Actually I may need to elaborate a bit on proposal 1... In JavaScript, you can't get parameter names unless you want to use regex and have a crazy edge-case-filled mess on your hands (plus, we'd have to figure out how to get the parameter names on both ends, not just on one end, of the network. Would get messy). I was basically saying instead of passing JavaScript's built-in ..args rest parameters around the papi, we could transform that array into an object that is pretty much just an object representation of the array (and we would have to make sure not to include key/value pairs for undefined values so the value doesn't just get converted to null like is happening now in arrays). So like this:

networkObject.doStuff(3, undefined, 'happy meal')

right now serializes to this:
['doStuff', 3, null, 'happy meal']

but would be something like this if we did proposal 1:
{ 'functionName': 'doStuff', '0': 3, '2': 'happy meal' } (note JSON doesn't support numbers as keys, so they are converted to strings)

I don't think this is as nice as you were thinking I was proposing. It does give us the functionName more cleanly. Side note: we could just change the api to do that without converting an array into an object haphazardly like { 'functionName': 'doStuff', 'args': [3, 'happy meal'] } (which we probably should do... That's a lot nicer than jamming the function name into the array like I decided to do for whatever reason. Our papi supports it; I just didn't write it that way. Maybe it was a tad bit easier to write or something). Of course, that doesn't solve the undefined issue, though; just a side note.

If we go with proposal 1, we wouldn't have to use a replacer and reviver to get it to work, but we would still be writing a bunch of code that transforms arrays so they serialize differently than they normally do. In my mind, it's pretty similar to proposal 2 in that disadvantage, but it doesn't have the significant advantage of working everywhere in the papi all the time and we never have to worry about this again. However, it seems that we need to consider what we want to do in C# in either case since it doesn't have a concept of undefined. Almost makes me wonder if we should just load null as undefined everywhere (like in the reviver) and stop using them as different values even in JS... 🤔

Created #642 to track this in case we don't decide on something soon.

---

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

Previously, lyonsil (Matt Lyons) wrote…

I generally like to make things obvious, and the difference between "webView" and "WebView" when I'm scanning code is pretty small. I'm happy with an I in front or anything else that makes this stand out. I'm not married to Type at the end by any means. I just don't want anyone working on the code to have to be on their "A game" whenever they open the code because of capitalization.

Imo this is something to discuss and follow up on like a style guide conversation or something. Shouldn't block this PR because this kind of thing is already in the code in a few places. I would like to make some kind of change, though.

[irahopkinson]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil and @tjcouch-sil)

---

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

Previously, lyonsil (Matt Lyons) wrote…

I like 1) more than 2) because, as you say it's vanilla. I think we need to "embrace the boring" more often when the boring works. Also, passing an argument containing all the parameters seems like something that is cleaner to handle when we rework the network protocol. Instead of an array of unnamed objects of unknown types, we can at least have names to reference for each of the parameters. I think if it's an object, we can probably make the serialization/deserialization of network object calls easier to handle in .NET. It also allows us to do some extra verification of arguments and gives us a non-breaking way to add parameters to functions on network objects.

I don't see a need to rush a change in for this right away. I think we can take a little time to breathe and consider.

I'd vote for doing 2 (as per the playground) in this PR and put the defaults back. Then make issue #642 about doing 1 which can be done at a later date. 2 could also be a follow-up PR to this one to keep things rolling but since the issue was caused by this PR the (simple) fix probably shouldn't be divorced from it to be rescheduled later.

[tjcouch-sil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson and @lyonsil)

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

I'd vote for doing 2 (as per the playground) in this PR and put the defaults back. Then make issue #642 about doing 1 which can be done at a later date. 2 could also be a follow-up PR to this one to keep things rolling but since the issue was caused by this PR the (simple) fix probably shouldn't be divorced from it to be rescheduled later.

I'd actually argue this problem has been latent the whole time this software has existed. Unless you have code that defaults args to the right thing on both sides (not possible for extensions, but not really the focus of this discussion), undefined goes to null instead of the default value. Matt just happened to stumble upon the issue.

The more I think about it, the more I like the idea of pretending like null doesn't exist in JavaScript and just using undefined (would need to revise a few apis) haha I would say the other way around if it weren't for the fact that null is super weird in JS 😂

If we were an all-JS application or if we worked with other languages that have this distinction, I'd probably want to continue distinguishing between the two where helpful but generally use undefined since a distinction between the two is helpful sometimes, but that's not our use case.

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson and @tjcouch-sil)

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

I'd actually argue this problem has been latent the whole time this software has existed. Unless you have code that defaults args to the right thing on both sides (not possible for extensions, but not really the focus of this discussion), undefined goes to null instead of the default value. Matt just happened to stumble upon the issue.

The more I think about it, the more I like the idea of pretending like null doesn't exist in JavaScript and just using undefined (would need to revise a few apis) haha I would say the other way around if it weren't for the fact that null is super weird in JS 😂

If we were an all-JS application or if we worked with other languages that have this distinction, I'd probably want to continue distinguishing between the two where helpful but generally use undefined since a distinction between the two is helpful sometimes, but that's not our use case.

Ah, I did misunderstand what proposal 1 is. I don't love that. 2 is better than that, but I was hoping we would be able to find something a little cleaner for serialization/deserialization purposes. Maybe not, though.

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

Imo this is something to discuss and follow up on like a style guide conversation or something. Shouldn't block this PR because this kind of thing is already in the code in a few places. I would like to make some kind of change, though.

Agreed this is a style guide conversation.