Send network events when network objects are created

[lyonsil]

Also update the npm stop script

---

This change is 

[lyonsil]

I decided to take this a step further and start down the path of publishing the functions available on network objects when they are registered. The C# side of this could use some work (added the "TODO" to highlight this), but doing that level of rework was definitely outside the scope of this ticket. That might come into play with #645 or maybe #193.

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

[lyonsil]

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

---

stop-processes.mjs line 6 at r1 (raw file):

// All processes with any of these terms in the command line will be killed
const searchTerms = ['electronmon', 'esbuild', 'nodemon', 'vite', 'webpack', 'extension-host'];

I noticed that there was often 1 dangling process after I stopped the platform in the debugger. This change gets rid of that dangling process.

[irahopkinson]

Thanks for taking it further than designated. I think that was a good call and also where you stopped going further.

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

---

src/shared/services/network-object.service.ts line 122 at r1 (raw file):

const hasKnown = (id: string): boolean => networkObjectRegistrations.has(id);

export type NetworkObjectDetails = {

Does this really need to be exported? I don't see it used anywhere else. If it does it might be better to move into src\shared\models\network-object.model.ts. Exported items should usually also have a JSDoc at least on each property.

---

src/shared/services/network-object.service.ts line 124 at r1 (raw file):

export type NetworkObjectDetails = {
  id: string;
  functions: string[];

This actually holds function names rather then the functions themselves. Either rename the property or clarify in the property JSDoc.

---

src/shared/services/network-object.service.ts line 270 at r1 (raw file):

function createNetworkObjectDetails(
  id: string,
  objectToShare: Record<string, unknown>,

FYI there is a clearer syntax (IMO) for this type since it allows you to name the key, e.g.:

objectToShare: { [property: string]: unknown },

Code quote:

Record<string, unknown>

---

src/shared/services/network-object.service.ts line 272 at r1 (raw file):

  objectToShare: Record<string, unknown>,
): NetworkObjectDetails {
  const objectFunctions = new Set<string>();

Minor: this is actually contains the function property names not the functions themselves. Consider renaming to objectProperties or objectFunctionNames.

Code quote:

objectFunctions

[tjcouch-sil]

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @irahopkinson and @lyonsil)

---

c-sharp/Messages/MessageEventNetworkObjectCreated.cs line 3 at r1 (raw file):

namespace Paranext.DataProvider.Messages;

public sealed class MessageEventObjectCreate : MessageEventGeneric<MessageEventObjectCreateContents>

Why is the name of this class so different than the name of the file? I think the file name reads well, but the class name is hard to understand. I wouldn't know what the class name is supposed to represent if I weren't already thinking about this PR. Could we consider renaming to the file name or finding another name?

---

src/shared/services/network-object.service.ts line 131 at r1 (raw file):

 * network object.
 */
const onDidCreateNetworkObjectEmitter =

I suggest considering renaming to onDidAddNetworkObject and such to match more closely with onDidAddWebView. Or maybe not; Just a suggestion to consider. Maybe we ought to think about unifying event terminology sometime.

---

src/shared/services/network-object.service.ts line 136 at r1 (raw file):

  );

export const onDidCreateNetworkObject = onDidCreateNetworkObjectEmitter.event;

I recommend adding a simple JSDoc describing what this event does (similar to onDidAddWebView)

---

src/shared/services/network-object.service.ts line 212 at r1 (raw file):

      if (!isString(key)) return undefined;
      // Don't create remote proxies for events
      if (key.startsWith('on')) return undefined;

Why prevent all functions starting with on in addition to just onDidDispose (which we manually set ourselves, hence the onDidDispose check here)? I'm wary of constraining network object properties beyond what is required/necessary - could we consider an alternative to preventing proxying on functions that still helps people to understand their events don't work on network objects natively? Maybe warn (seems a bit heavy-handed...)? Add to the network object documentation to clarify that you can't do events across the network without adding a createLocalObjectToProxy to the network-object.service.ts get call? Maybe just documenting more carefully is best. I just don't want to prevent people from naming functions what they want.

I think the onDidDispose check that was here previously could possibly be changed to throw an error. I don't think there should be any circumstances where that should get hit if I'm thinking about this correctly.

(Mind wandering a bit) Maybe we should consider making some kind of standardization for on functions! So then all on functions must be events, and our proxies assume they are so. I dunno; that part is out of scope for this issue anyway haha maybe we can consider something like that when we're looking at reworking network objects.

---

src/shared/services/network-object.service.ts line 270 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

FYI there is a clearer syntax (IMO) for this type since it allows you to name the key, e.g.:

objectToShare: { [property: string]: unknown },

What Ira showed is called an index signature. I also vote for that syntax.

---

src/shared/services/network-object.service.ts line 272 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Minor: this is actually contains the function property names not the functions themselves. Consider renaming to objectProperties or objectFunctionNames.

I agree with this distinction in the few places Ira noted throughout this PR

---

src/shared/services/network-object.service.ts line 272 at r1 (raw file):

  objectToShare: Record<string, unknown>,
): NetworkObjectDetails {
  const objectFunctions = new Set<string>();

I believe this is attempting to do essentially the same thing as the util function getAllObjectFunctionNames but is more thorough than the util function. Would you mind considering moving this code to the util function? I think it would be best to leave constructor and such in there and do the filtering out in here still.

---

src/shared/services/network-object.service.ts line 472 at r1 (raw file):

        getNetworkObjectRequestType(id, NetworkObjectRequestSubtype.Get),
        () =>
          Promise.resolve([

Do you want to put the network object functions on here to match current C# functionality? Maybe you could then move this TODO into the remote proxy since it's no longer relevant here.

I think it would make sense to make this an object containing function names (maybe even the same thing as the network object details event contents thing?), but it's probably worth considering such a change as part of our larger network object conversation.

---

c-sharp/Messages/MessageEventNetworkObjectDisposed.cs line 3 at r1 (raw file):

namespace Paranext.DataProvider.Messages;

public sealed class MessageEventObjectDispose : MessageEventGeneric<string>

Seems this class name doesn't have a d at the end whereas the file does - is this significant? I don't know the norm for file and class names in C#, but it seems strange to name them differently in my mind when one file represents one class.

[tjcouch-sil]

Thanks for the great work on this! A few questions and such.

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

[tjcouch-sil]

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

---

src/shared/services/network-object.service.ts line 122 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Does this really need to be exported? I don't see it used anywhere else. If it does it might be better to move into src\shared\models\network-object.model.ts. Exported items should usually also have a JSDoc at least on each property.

Since it is the event type of the onDidCreateNetworkObject event, I'd say it makes sense to export. It is not being used at the moment, but it will be one day. I also suggest moving it to a model file.

---

src/shared/services/network-object.service.ts line 472 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Do you want to put the network object functions on here to match current C# functionality? Maybe you could then move this TODO into the remote proxy since it's no longer relevant here.

I think it would make sense to make this an object containing function names (maybe even the same thing as the network object details event contents thing?), but it's probably worth considering such a change as part of our larger network object conversation.

Now that I have seen PR #652 about making network objects discoverable, I'm leaning more and more toward thinking the whole NetworkObjectDetails should be returned from get on network objects (instead of just the function names array) and stored in the network object status service. It seems the data type that holds information about network objects (currently just string[] of function names) is getting spread around a number of different places and may get a bit difficult to keep track of when we want to make updates or changes. Unifying it in all the places will make it easy to keep track of where it all is and where to update it (and making it an object allows us to expand upon it in the future). Not a requirement, but I am suggesting this more strongly now that I see a bigger picture.

[lyonsil]

Reviewable status: 9 of 18 files reviewed, 6 unresolved discussions (waiting on @irahopkinson, @lyonsil, and @tjcouch-sil)

---

src/shared/services/network-object.service.ts line 122 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Since it is the event type of the onDidCreateNetworkObject event, I'd say it makes sense to export. It is not being used at the moment, but it will be one day. I also suggest moving it to a model file.

I moved this to network-object.model

---

src/shared/services/network-object.service.ts line 124 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This actually holds function names rather then the functions themselves. Either rename the property or clarify in the property JSDoc.

Good point, I updated this to functionNames.

---

src/shared/services/network-object.service.ts line 131 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I suggest considering renaming to onDidAddNetworkObject and such to match more closely with onDidAddWebView. Or maybe not; Just a suggestion to consider. Maybe we ought to think about unifying event terminology sometime.

I was trying to align with Dispose since we have onDidDisposeNetworkObject a few lines later in this file. Add and Dispose aren't really opposites. Create and Dispose are closer and normal terminology in other contexts. I could also see Construct and Dispose as a possibility. I don't really like Add and Dispose being paired together, though. If we change Dispose to Remove then it would seem more natural. Having said this, if you really want Add and Dispose paired together I can do so.

---

src/shared/services/network-object.service.ts line 136 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I recommend adding a simple JSDoc describing what this event does (similar to onDidAddWebView)

Done - good idea

---

src/shared/services/network-object.service.ts line 212 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Why prevent all functions starting with on in addition to just onDidDispose (which we manually set ourselves, hence the onDidDispose check here)? I'm wary of constraining network object properties beyond what is required/necessary - could we consider an alternative to preventing proxying on functions that still helps people to understand their events don't work on network objects natively? Maybe warn (seems a bit heavy-handed...)? Add to the network object documentation to clarify that you can't do events across the network without adding a createLocalObjectToProxy to the network-object.service.ts get call? Maybe just documenting more carefully is best. I just don't want to prevent people from naming functions what they want.

I think the onDidDispose check that was here previously could possibly be changed to throw an error. I don't think there should be any circumstances where that should get hit if I'm thinking about this correctly.

(Mind wandering a bit) Maybe we should consider making some kind of standardization for on functions! So then all on functions must be events, and our proxies assume they are so. I dunno; that part is out of scope for this issue anyway haha maybe we can consider something like that when we're looking at reworking network objects.

Exposing a proxy to an event handler (which is what was happening before this change) doesn't make a lot of sense for what we're trying to do in the platform. (When your event locally fires, run this function in your process because I asked you to.) I don't any way to identify events other than the naming convention. Do you have another suggestion for identifying them?

I can add documentation about this point if you don't have a way to make this determination cleanly.

---

src/shared/services/network-object.service.ts line 270 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

What Ira showed is called an index signature. I also vote for that syntax.

Done

---

src/shared/services/network-object.service.ts line 272 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I believe this is attempting to do essentially the same thing as the util function getAllObjectFunctionNames but is more thorough than the util function. Would you mind considering moving this code to the util function? I think it would be best to leave constructor and such in there and do the filtering out in here still.

Moved most of the logic to getAllObjectFunctionNames in utils.

---

src/shared/services/network-object.service.ts line 272 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I agree with this distinction in the few places Ira noted throughout this PR

Changed to objectFunctionNames

---

src/shared/services/network-object.service.ts line 472 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Now that I have seen PR #652 about making network objects discoverable, I'm leaning more and more toward thinking the whole NetworkObjectDetails should be returned from get on network objects (instead of just the function names array) and stored in the network object status service. It seems the data type that holds information about network objects (currently just string[] of function names) is getting spread around a number of different places and may get a bit difficult to keep track of when we want to make updates or changes. Unifying it in all the places will make it easy to keep track of where it all is and where to update it (and making it an object allows us to expand upon it in the future). Not a requirement, but I am suggesting this more strongly now that I see a bigger picture.

I made the change in .NET because it was basically free. I didn't set out intending to change it, but I realized that I had all the data already at the right place in the same file at the right time, so it was trivial. I would have to do some more thinking here and reordering of calls, and I was already going a bit past the point of the ticket. I assumed more changes would all be part of the larger conversation, too, around the network protocol and multi-threading. I REALLY would like us to get to the point where we are both happy with the protocol, how it is handled in TS and C#, and we have some documentation about it.

Let's say that I did add it here despite everything I mentioned above. It wouldn't change how the network object status service works, because that is only looking for events raised when the objects get registered, not the registration calls themselves. The function names will just be thrown away during the registration call by the network service. Honestly I'd rather revert the 1-2 lines of code in C# that sends the function names than mess with it here any further.

---

c-sharp/Messages/MessageEventNetworkObjectCreated.cs line 3 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Why is the name of this class so different than the name of the file? I think the file name reads well, but the class name is hard to understand. I wouldn't know what the class name is supposed to represent if I weren't already thinking about this PR. Could we consider renaming to the file name or finding another name?

Good catch - I renamed this message and the related Dispose message to align and match the class names. The file names and class names got adjusted a few times while I was doing this work, and I forgot to make them match at the end. It's not a huge issue, but it is nice to catch.

---

c-sharp/Messages/MessageEventNetworkObjectDisposed.cs line 3 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Seems this class name doesn't have a d at the end whereas the file does - is this significant? I don't know the norm for file and class names in C#, but it seems strange to name them differently in my mind when one file represents one class.

Good catch. I made these line up.

[irahopkinson]

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

[tjcouch-sil]

Thanks for the updates! Just a couple optional suggestions, and you're good to go.

Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lyonsil)

---

src/shared/services/network-object.service.ts line 122 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I moved this to network-object.model

Thanks!

---

src/shared/services/network-object.service.ts line 131 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I was trying to align with Dispose since we have onDidDisposeNetworkObject a few lines later in this file. Add and Dispose aren't really opposites. Create and Dispose are closer and normal terminology in other contexts. I could also see Construct and Dispose as a possibility. I don't really like Add and Dispose being paired together, though. If we change Dispose to Remove then it would seem more natural. Having said this, if you really want Add and Dispose paired together I can do so.

Sounds good :) note it looks like you're blocking on this comment - not sure if that will prevent merging, but figured I'd let you know since I almost didn't notice and probably wouldn't be looking for that if I were you.

---

src/shared/services/network-object.service.ts line 136 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Done - good idea

Thanks!

---

src/shared/services/network-object.service.ts line 212 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Exposing a proxy to an event handler (which is what was happening before this change) doesn't make a lot of sense for what we're trying to do in the platform. (When your event locally fires, run this function in your process because I asked you to.) I don't any way to identify events other than the naming convention. Do you have another suggestion for identifying them?

I can add documentation about this point if you don't have a way to make this determination cleanly.

Was there a specific event handler that was being proxied? The only event handler I can think of that is guaranteed to be on every single network object is onDidDispose, and we were already handling that previously.

There is currently no way to tell if a function is an event to my knowledge. We might be able to add a property to the event function in papi-event-emitter.model.ts and then check for that property and keep track of the functions that are events by passing the info around between processes on the NetworkObjectDetails. But that's a lot of work that probably isn't worth doing right now. Essentially there is no built-in inherent way to tell a difference since an event is just a function.

As you noted, events need to be local to each process in order to work properly, so they indeed shouldn't be proxied like normal network object functions. But we just don't have enough inherent information in function names to know which is which other than onDidDispose. My preference is to revert the code to the way it was before since we can't guarantee that a function that starts with on is an event. I'd prefer to do less restricting on assumptions and let people do more of what they want and just mention in the documentation that events don't work without some extra work (getting them into the createLocalObjectToProxy function on each process).

Either way you decide to do it (leave this code as-is right now or revert it), it would be good to document it in the JSDocs somewhere. I will unblock this comment and let you put this code in or revert as you choose, but I would highly recommend making a comment either way.

We could also make an issue to get events working on network objects if we want - we could get creative with it and probably make something work pretty nicely.

(Also note you're blocking on this discussion as well)

---

src/shared/services/network-object.service.ts line 272 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Moved most of the logic to getAllObjectFunctionNames in utils.

Looks good :) thank you very much!

---

src/shared/services/network-object.service.ts line 472 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I made the change in .NET because it was basically free. I didn't set out intending to change it, but I realized that I had all the data already at the right place in the same file at the right time, so it was trivial. I would have to do some more thinking here and reordering of calls, and I was already going a bit past the point of the ticket. I assumed more changes would all be part of the larger conversation, too, around the network protocol and multi-threading. I REALLY would like us to get to the point where we are both happy with the protocol, how it is handled in TS and C#, and we have some documentation about it.

Let's say that I did add it here despite everything I mentioned above. It wouldn't change how the network object status service works, because that is only looking for events raised when the objects get registered, not the registration calls themselves. The function names will just be thrown away during the registration call by the network service. Honestly I'd rather revert the 1-2 lines of code in C# that sends the function names than mess with it here any further.

The difference I am suggesting regarding the network object status service is that it would change to be handling NetworkObjectDetails instead of just string[]. Like this:

const networkObjectIDsToFunctions = new Map<string, string[]>();

Would become this:

const networkObjectIDsToFunctions = new Map<string, NetworkObjectDetails>();

It would make it easier to understand and keep track of where all the network object info is going because it's all related to NetworkObjectDetails now instead of being NetworkObjectDetails in some places and string[] in other places where you can't easily Find all References. But it's not a huge deal right now; just a suggestion. I would like to consider this change later during rework, so we can just keep an eye out for when the time is right if you understandably want to pass on making these changes now.

---

c-sharp/Messages/MessageEventNetworkObjectCreated.cs line 3 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Good catch - I renamed this message and the related Dispose message to align and match the class names. The file names and class names got adjusted a few times while I was doing this work, and I forgot to make them match at the end. It's not a huge issue, but it is nice to catch.

I think it still feels a bit opaque to me, but it's not a big deal anymore. Feel free to resolve if you want.

I think I liked MessageEventNetworkObjectCreated since it's clear that it's talking about a network object. But I appreciate that there is at least some implication that we're talking about a network object already since it's a network event. 🤷‍♂️

---

c-sharp/Messages/MessageEventNetworkObjectDisposed.cs line 3 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Good catch. I made these line up.

Thanks!

[lyonsil]

Reviewable status: all files reviewed, 2 unresolved discussions

---

src/shared/services/network-object.service.ts line 131 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Sounds good :) note it looks like you're blocking on this comment - not sure if that will prevent merging, but figured I'd let you know since I almost didn't notice and probably wouldn't be looking for that if I were you.

Yes I made the comment blocking on purpose to hopefully make it clear I was waiting on what to do based on what your response would be. Maybe it wasn't so clear! 😆

[lyonsil]

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

---

src/shared/services/network-object.service.ts line 212 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Was there a specific event handler that was being proxied? The only event handler I can think of that is guaranteed to be on every single network object is onDidDispose, and we were already handling that previously.

There is currently no way to tell if a function is an event to my knowledge. We might be able to add a property to the event function in papi-event-emitter.model.ts and then check for that property and keep track of the functions that are events by passing the info around between processes on the NetworkObjectDetails. But that's a lot of work that probably isn't worth doing right now. Essentially there is no built-in inherent way to tell a difference since an event is just a function.

As you noted, events need to be local to each process in order to work properly, so they indeed shouldn't be proxied like normal network object functions. But we just don't have enough inherent information in function names to know which is which other than onDidDispose. My preference is to revert the code to the way it was before since we can't guarantee that a function that starts with on is an event. I'd prefer to do less restricting on assumptions and let people do more of what they want and just mention in the documentation that events don't work without some extra work (getting them into the createLocalObjectToProxy function on each process).

Either way you decide to do it (leave this code as-is right now or revert it), it would be good to document it in the JSDocs somewhere. I will unblock this comment and let you put this code in or revert as you choose, but I would highly recommend making a comment either way.

We could also make an issue to get events working on network objects if we want - we could get creative with it and probably make something work pretty nicely.

(Also note you're blocking on this discussion as well)

onDidAddWebView showed up, too, not just onDidDispose. That's why I widened out the exclusion. I imagine other events could show up in the future unintentionally. This restriction seems pretty benign and making catching new events automatic as long as we follow the naming convention. We're already enforcing name conventions with getXYZ and setXYZ with data providers, too.

I don't think trying to get events working on network objects is desirable. At least I can't think of a good reason for doing so, and I can think of some negative ones. It seems like something that could circumvent security work done for extensions, too. If we come up with a real use case we could always re-investigate.

I agree commenting is a good idea for this. I added a few sentences to the description of the network object service itself.

---

src/shared/services/network-object.service.ts line 472 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

The difference I am suggesting regarding the network object status service is that it would change to be handling NetworkObjectDetails instead of just string[]. Like this:

const networkObjectIDsToFunctions = new Map<string, string[]>();

Would become this:

const networkObjectIDsToFunctions = new Map<string, NetworkObjectDetails>();

It would make it easier to understand and keep track of where all the network object info is going because it's all related to NetworkObjectDetails now instead of being NetworkObjectDetails in some places and string[] in other places where you can't easily Find all References. But it's not a huge deal right now; just a suggestion. I would like to consider this change later during rework, so we can just keep an eye out for when the time is right if you understandably want to pass on making these changes now.

I'm going to put off further changes on this for the moment. When I pick up the network object status service again shortly, I will take this feedback into account there, though.

[irahopkinson]

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