Update the network object service to modify the objects passed in

[lyonsil]

This change is 

[tjcouch-sil]

This is great work!! Thanks for all the hard work on this. I'm really looking forward to having these major improvements to network objects and data providers. I added a couple blocking comments as well as many optional comments. Please consider and make adjustments or let me know your thoughts and don't adjust :) I hope this is helpful!

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

---

src/extension-host/extension-host.ts line 64 at r2 (raw file):

(async () => {
  const testEHInfo = await networkObjectService.set('test-extension-host', {

Remove "info" from name

---

src/extension-host/extension-host.ts line 84 at r2 (raw file):

setTimeout(async () => {
  let testMainInfo = await networkObjectService.get<{

Remove "info" from name

---

src/renderer/hooks/papi-hooks/use-data-provider.hook.ts line 54 at r2 (raw file):

): T | undefined;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function useDataProvider<T extends IDataProvider<any, any, any>>(

It seems this function returns DataProviderInfo<any, any, any> now. Please change this in all the function signatures and comments about signatures.

---

src/renderer/hooks/papi-hooks/use-data-provider.hook.ts line 90 at r2 (raw file):

  return dataProviderInfo && !isDisposed
    ? // Type assert here - the user of this hook must make sure to provide the correct type
      (dataProviderInfo as unknown as T)

Unless there is no other way to accomplish the typing we want without type asserting to unknown, we should avoid it as it entirely removes all write-time type safety from TypeScript (asserting directly without unknown makes sure the types are "compatible"). It seems we can avoid unknown here by removing the assertion here on line 90 and mentioning that dataProviderInfo can alternatively be undefined as soon as we retrieve it from dataProviderService.get, adding the assertion (and the explanatory comment) up on line 69:

dataProviderSource ? dataProviderService.get(dataProviderSource) as Promise<T | undefined> : undefined;

That way, we do the type assert that we know should be happening as soon as we can. Previously, type asserting up on line 69 was not easy/possible (don't remember which) because the actual object with type T was wrapped in another object. However, now that we have the object itself, we can type assert to let TypeScript know asap that we know the resolved value from dataProviderService.get is either T or undefined.

---

src/shared/models/network-object-info.model.ts line 6 at r2 (raw file):

/**
 * An object of this type is returned from NetworkObjectService.get.

I think it would be useful to include a description comment of what this type is supposed to represent, not just where it is used. Something similar to This object is retrievable by any process using NetworkObjectService.get, and any process can call its methods.

---

src/shared/models/network-object-info.model.ts line 14 at r2 (raw file):

/**
 * An object of this type is returned from NetworkObjectService.set.

I think it would be useful to include a description comment of what this type is supposed to represent, not just where it is used. Something similar to This object is set up by NetworkObjectService.set to be retrievable by any process using NetworkObjectService.get, and any process can call its methods. However, only the code that calls NetworkObjectService.setcan call itsdispose` method.

---

src/shared/models/network-object-info.model.ts line 24 at r2 (raw file):

/**
 * An object of this type is passed into NetworkObjectService.set. Its properties related to
 * disposal will be changed.

It seems helpful to be more specific here - maybe mention particularly that dispose will be layered over so it is still called.

---

src/shared/models/network-object-info.model.ts line 30 at r2 (raw file):

 * functions' arguments and returns must be serializable in order to be called across processes.
 */
export type NetworkableObject<T> = Partial<DisposableNetworkObject<T>> & {

I think it would be useful to include a description comment of what this type is supposed to represent, not just where it is used. Something similar to Object that is able to be shared on the network from the old comments.

---

src/shared/models/network-object-info.model.ts line 31 at r2 (raw file):

 */
export type NetworkableObject<T> = Partial<DisposableNetworkObject<T>> & {
  onDidDispose: never;

Maybe consider making a JSDoc here that mentions onDidDispose is created as part of making a NetworkObject and therefore it is not allowed to exist on a NetworkableObject so people know what's going on here a bit better.

---

src/shared/models/network-object-info.model.ts line 47 at r2 (raw file):

 * If a network object with the provided id exists remotely but has not been set up
 * to be used on this process, this function is run in NetworkObjectService.get, and the returned object is used as a base on which to set up a
 * NetworkObject for use on this process. This is useful for setting up network events on a network object.

I also think it is useful to mention that this is especially useful for network events. Do you think it is not valuable to mention this?

---

src/shared/models/network-object-info.model.ts line 48 at r2 (raw file):

* @returns the local object to proxy into a network object.
 */
export type LocalObjectToProxyCreator<T> = (

I could be missing something, but isn't it still useful to mark this as T extends NetworkableObject? Maybe T extends NetworkableObject<unknown> or T extends NetworkableObject<Record<string, unknown/any>>? Hard to tell from Reviewable what would be best or if I am missing something.

---

src/shared/models/network-object-info.model.ts line 51 at r2 (raw file):

 * @param id id of the network object to get
 *
 * @param networkObjectContainer holds a reference to the final proxied network object that is created (yes, this is self-referencing).

I think the comments removed from this function are important for explaining what's going on with the container thing, particularly old line numbers 51-54. Do you think the comments removed from this function type were unhelpful?

---

src/shared/services/data-provider.service.ts line 37 at r2 (raw file):

/** Gets the id for the data provider network object with the given name */
const getDataProviderObjectId = (providerName: string) => `${providerName}-${DATA_PROVIDER_LABEL}`;

In registerEngine, useDataProvider, and useData, this is called dataType. In get, it is called dataProviderName. I recommend we tighten this up and use the same variable name everywhere. I suggest dataType since that is what a data provider's string label represents and what people want to get out of a data provider. Like usfm or usx or biblicalTerms or something. People might think "I want to get usfm data, so I'll get the data provider for the data type usfm". However, I certainly appreciate that it also makes a lot of sense to think about it more in terms of a data provider's name. Either way is fine, but I still recommend we use the same name everyywhere for clarity and consistency.

---

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

const mutexMap: MutexMap = new MutexMap();

/** Remote proxy means a proxy object referencing something on the other side of the network */

Might be helpful to describe what we are creating here. Something like "This proxy allows processes that do not own the network object to call the remote network object's functions" or something?

---

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

  });

/** Local proxy means a proxy object in the same process used by something that didn't set the object */

Might be helpful to describe what we are creating here. Something like "This proxy allows other code in the same process to call the local network object directly" or something?

---

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

    const networkObjectRegistration = networkObjectRegistrations.get(id);
    if (networkObjectRegistration)
      return Promise.resolve(networkObjectRegistration.networkObject as NetworkObject<T>);

Don't need to wrap this in Promise.resolve since this is already asynchronous.

---

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

 * Set up an object to be shared on the network.
 * @param id ID of the object to share on the network. All processes must use this ID to look it up.
 * @param objectToShare The object to set up as a network object. Its `dispose` and `onDidDispose` properties will be written/overwritten.

May be worth specifying that dispose is layered over and still called while onDidDispose cannot exist on the object`

[lyonsil]

Reviewable status: 9 of 12 files reviewed, 3 unresolved discussions (waiting on @tjcouch-sil)

---

src/extension-host/extension-host.ts line 64 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Remove "info" from name

done

---

src/extension-host/extension-host.ts line 84 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Remove "info" from name

done

---

src/shared/models/network-object-info.model.ts line 6 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think it would be useful to include a description comment of what this type is supposed to represent, not just where it is used. Something similar to This object is retrievable by any process using NetworkObjectService.get, and any process can call its methods.

I'm trying to determine a single place to put a more complete description of how network objects work and avoid splitting the docs in many places or duplicating content. See what you think of this next set of changes.

---

src/shared/models/network-object-info.model.ts line 14 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think it would be useful to include a description comment of what this type is supposed to represent, not just where it is used. Something similar to This object is set up by NetworkObjectService.set to be retrievable by any process using NetworkObjectService.get, and any process can call its methods. However, only the code that calls NetworkObjectService.setcan call itsdispose` method.

I'm trying to determine a single place to put a more complete description of how network objects work and avoid splitting the docs in many places or duplicating content. See what you think of this next set of changes.

---

src/shared/models/network-object-info.model.ts line 24 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

It seems helpful to be more specific here - maybe mention particularly that dispose will be layered over so it is still called.

I'm trying to determine a single place to put a more complete description of how network objects work and avoid splitting the docs in many places or duplicating content. See what you think of this next set of changes.

---

src/shared/models/network-object-info.model.ts line 30 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think it would be useful to include a description comment of what this type is supposed to represent, not just where it is used. Something similar to Object that is able to be shared on the network from the old comments.

I'm trying to determine a single place to put a more complete description of how network objects work and avoid splitting the docs in many places or duplicating content. See what you think of this next set of changes.

---

src/shared/models/network-object-info.model.ts line 31 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Maybe consider making a JSDoc here that mentions onDidDispose is created as part of making a NetworkObject and therefore it is not allowed to exist on a NetworkableObject so people know what's going on here a bit better.

I'm trying to determine a single place to put a more complete description of how network objects work and avoid splitting the docs in many places or duplicating content. See what you think of this next set of changes.

---

src/shared/models/network-object-info.model.ts line 47 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I also think it is useful to mention that this is especially useful for network events. Do you think it is not valuable to mention this?

I'm trying to determine a single place to put a more complete description of how network objects work and avoid splitting the docs in many places or duplicating content. See what you think of this next set of changes.

---

src/shared/models/network-object-info.model.ts line 48 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I could be missing something, but isn't it still useful to mark this as T extends NetworkableObject? Maybe T extends NetworkableObject<unknown> or T extends NetworkableObject<Record<string, unknown/any>>? Hard to tell from Reviewable what would be best or if I am missing something.

It used to be T extends NetworkableObject, but NetworkableObject was an empty interface, so that didn't provide much meaning. I think I removed it early on for that reason. I am adding it back now since NetworkableObject has something inside of it.

---

src/shared/models/network-object-info.model.ts line 51 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think the comments removed from this function are important for explaining what's going on with the container thing, particularly old line numbers 51-54. Do you think the comments removed from this function type were unhelpful?

Good point - For a while I tried removing the container altogether and just provide the object without using a container. I think that's when I removed those comments. Since a container was put back, I should reintroduce the comments.

---

src/shared/services/data-provider.service.ts line 37 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

In registerEngine, useDataProvider, and useData, this is called dataType. In get, it is called dataProviderName. I recommend we tighten this up and use the same variable name everywhere. I suggest dataType since that is what a data provider's string label represents and what people want to get out of a data provider. Like usfm or usx or biblicalTerms or something. People might think "I want to get usfm data, so I'll get the data provider for the data type usfm". However, I certainly appreciate that it also makes a lot of sense to think about it more in terms of a data provider's name. Either way is fine, but I still recommend we use the same name everyywhere for clarity and consistency.

Yeah I missed some references, and I agree consistency is important here. I moved to "name" from "type" because the values passed in aren't really types in the sense of TYPEscript. They are names of objects that return data, and that data can have different types. Also more than one data source could return the same data type. If we have an actual type named usfm we could name the data source usfm, but we could also have a source named paratext and another named some-other-usfm-source that both return usfm data.

If we want to go with the network object terminology, we could call these IDs instead of names. I'd be fine with that, too. It took me a little while to realize that "type" didn't really mean "type" which is why I changed it. Perhaps we can talk with others at stand up to settle on the terminology? If the group says "type" is best, then I'll switch it back to how it was.

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

Might be helpful to describe what we are creating here. Something like "This proxy allows processes that do not own the network object to call the remote network object's functions" or something?

I changed the wording. Hopefully this is better!

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

Might be helpful to describe what we are creating here. Something like "This proxy allows other code in the same process to call the local network object directly" or something?

I changed the wording. Hopefully this is better!

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

Don't need to wrap this in Promise.resolve since this is already asynchronous.

done

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

May be worth specifying that dispose is layered over and still called while onDidDispose cannot exist on the object`

Reworked the docs

[tjcouch-sil]

The comment rework looks great! Thanks for the hard work on it :) Just a couple more little things

Reviewed 1 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lyonsil)

---

src/shared/models/network-object-info.model.ts line 47 at r2 (raw file):

Previously, lyonsil wrote…

I'm trying to determine a single place to put a more complete description of how network objects work and avoid splitting the docs in many places or duplicating content. See what you think of this next set of changes.

I think this event remark may not have gotten coverage in the comment rework. See below in networkObjectService.get's comment - I think it may fit well in there? Or maybe it makes sense to put it here to carry it with this type. Dunno; feel free to put it wherever seems best to you.

---

src/shared/models/network-object-info.model.ts line 48 at r2 (raw file):

Previously, lyonsil wrote…

It used to be T extends NetworkableObject, but NetworkableObject was an empty interface, so that didn't provide much meaning. I think I removed it early on for that reason. I am adding it back now since NetworkableObject has something inside of it.

I don't see these changes yet; assuming there are type changes to come. Thanks!

---

src/shared/services/data-provider.service.ts line 37 at r2 (raw file):

Previously, lyonsil wrote…

Yeah I missed some references, and I agree consistency is important here. I moved to "name" from "type" because the values passed in aren't really types in the sense of TYPEscript. They are names of objects that return data, and that data can have different types. Also more than one data source could return the same data type. If we have an actual type named usfm we could name the data source usfm, but we could also have a source named paratext and another named some-other-usfm-source that both return usfm data.

If we want to go with the network object terminology, we could call these IDs instead of names. I'd be fine with that, too. It took me a little while to realize that "type" didn't really mean "type" which is why I changed it. Perhaps we can talk with others at stand up to settle on the terminology? If the group says "type" is best, then I'll switch it back to how it was.

I think you have a great point! I think going with id or dataProviderName or something along those lines makes sense. Thanks for your insightful feedback and for being thoughtful about how to improve it!

---

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

 * @param createLocalObjectToProxy Function that creates an object that the network object proxy
 * will be based upon. The object this function creates cannot have an `onDidDispose` property
 * defined because we will ignore it and overwrite it while setting up the proxy.

We are not ignoring the onDidDispose here, right? overrideOnDidDispose throws an error if the networkable object has it.

Also, I think the comment I asked to be included about that this LocalObjectToProxyCreator is especially useful for creating events on network objects didn't get re-introduced. Think it might be best to go here with these new comment reworks?

---

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

    // Setup onDidDispose so that services will know when the proxy is dead
    const eventEmitter = new PapiEventEmitter<void>();
    overrideOnDidDispose(baseObject, remoteProxy.proxy, eventEmitter.event);

Seeing that we have to put two different objects in here makes me wonder if the undefined check and set cache busting we did on the data provider proxy might be worth doing on the remote object proxy as well. I wonder if that will fix this problem (and would probably help prevent bugs in the future). Think we could at least put a TODO: in the remote object proxy to remind ourselves to add the cache busting in the future? Unless you want to do it now; that's fine for me :) just don't want to keep holding this up forever

---

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

 * @param id ID of the object to share on the network. All processes must use this ID to look it up.
 * @param objectToShare The object to set up as a network object. It will have an event named
 * `onDidDispose` added to its properties. If the object already contained a `dispose` function, a

May also be worth mentioning here that if objectToShare has onDidDispose, it will throw an error.

[lyonsil]

Thanks for your helpful comments and hands on working with me on this. This should cover everything (or at least very close to everything) we talked about and you commented on. Maybe one very small thing left.

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

---

src/renderer/hooks/papi-hooks/use-data-provider.hook.ts line 54 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

It seems this function returns DataProviderInfo<any, any, any> now. Please change this in all the function signatures and comments about signatures.

done

---

src/renderer/hooks/papi-hooks/use-data-provider.hook.ts line 90 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Unless there is no other way to accomplish the typing we want without type asserting to unknown, we should avoid it as it entirely removes all write-time type safety from TypeScript (asserting directly without unknown makes sure the types are "compatible"). It seems we can avoid unknown here by removing the assertion here on line 90 and mentioning that dataProviderInfo can alternatively be undefined as soon as we retrieve it from dataProviderService.get, adding the assertion (and the explanatory comment) up on line 69:

dataProviderSource ? dataProviderService.get(dataProviderSource) as Promise<T | undefined> : undefined;

That way, we do the type assert that we know should be happening as soon as we can. Previously, type asserting up on line 69 was not easy/possible (don't remember which) because the actual object with type T was wrapped in another object. However, now that we have the object itself, we can type assert to let TypeScript know asap that we know the resolved value from dataProviderService.get is either T or undefined.

done

---

src/shared/models/network-object-info.model.ts line 47 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think this event remark may not have gotten coverage in the comment rework. See below in networkObjectService.get's comment - I think it may fit well in there? Or maybe it makes sense to put it here to carry it with this type. Dunno; feel free to put it wherever seems best to you.

Can you restate the comment enhancement you'd like to see? I'm not quite following your original comment now, and it might be due to the changes that already went in.

---

src/shared/models/network-object-info.model.ts line 48 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I don't see these changes yet; assuming there are type changes to come. Thanks!

done - sorry about that

---

src/shared/services/data-provider.service.ts line 37 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think you have a great point! I think going with id or dataProviderName or something along those lines makes sense. Thanks for your insightful feedback and for being thoughtful about how to improve it!

I started going down the path of usingid, but that got a little confusing because of getDataProviderObjectId. Rather than having a data provider ID that is used to generate a data provider object ID leading to the question "Which kind of ID am I looking at?", I went forward with "name". So a data provider name is used to generate a data provider object ID which is used to create and register the network object.

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

We are not ignoring the onDidDispose here, right? overrideOnDidDispose throws an error if the networkable object has it.

Also, I think the comment I asked to be included about that this LocalObjectToProxyCreator is especially useful for creating events on network objects didn't get re-introduced. Think it might be best to go here with these new comment reworks?

fixed - also, onDidDispose being disallowed is enforced now by tsc, not just at runtime. It's hard to see exactly which previous comment you were referencing, but I think what is there now covers everything we discussed. If not, please flag it again in the latest code.

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

Seeing that we have to put two different objects in here makes me wonder if the undefined check and set cache busting we did on the data provider proxy might be worth doing on the remote object proxy as well. I wonder if that will fix this problem (and would probably help prevent bugs in the future). Think we could at least put a TODO: in the remote object proxy to remind ourselves to add the cache busting in the future? Unless you want to do it now; that's fine for me :) just don't want to keep holding this up forever

I added the cache buster as it could help avoid future strange bugs. It doesn't get rid of the possibility of hitting this bug again, but I changed the get proxy logic to avoid it instead of passing two objects to overrideOnDidDispose to lessen potential problems.

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

May also be worth mentioning here that if objectToShare has onDidDispose, it will throw an error.

done

[tjcouch-sil]

Very close!! Thank you for the hard work!

Reviewed 10 of 11 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lyonsil)

---

src/renderer/hooks/papi-hooks/use-data-provider.hook.ts line 91 at r6 (raw file):

  // If we had to get a data provider, return it if it is not disposed
  return dataProviderInfo && !isDisposed
    ? // Type assert here - the user of this hook must make sure to provide the correct type

Please move this type assertion comment up above where the assertion is now

---

src/shared/models/disposal-interface.ts line 1 at r6 (raw file):

import { PapiEvent } from '@shared/models/papi-event.model';

This file name does not follow our naming conventions. Consider disposal-interface.model.ts or something. Maybe consult Ira :)

---

src/shared/models/network-object-info.model.ts line 47 at r2 (raw file):

Previously, lyonsil wrote…

Can you restate the comment enhancement you'd like to see? I'm not quite following your original comment now, and it might be due to the changes that already went in.

I will do that in the networkobjectService.get area :)

---

src/shared/models/network-object-info.model.ts line 48 at r2 (raw file):

Previously, lyonsil wrote…

done - sorry about that

No problem!! Just wanted to be sure to follow up on this

---

src/shared/services/data-provider.service.ts line 37 at r2 (raw file):

Previously, lyonsil wrote…

I started going down the path of usingid, but that got a little confusing because of getDataProviderObjectId. Rather than having a data provider ID that is used to generate a data provider object ID leading to the question "Which kind of ID am I looking at?", I went forward with "name". So a data provider name is used to generate a data provider object ID which is used to create and register the network object.

Gotcha. Thanks for looking into it. Would you mind either updating the places where dataType is used elsewhere or filing a new issue to change the names please? Just want to be consistent. So in useDataProvider and useData, for example, we need to change the parameter name from dataType to providerName. (dataType is an overloaded parameter name in those two files in particular, so the actual function code shouldn't change.)

---

src/shared/services/data-provider.service.ts line 321 at r6 (raw file):

 */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
async function get<T extends IDataProvider<any, any, any> & Record<string, unknown>>(

Do we need to add Record here? Why? It is expected that extensions that define data providers will also define a data provider type that people can use to call custom methods on those data providers. I believe adding Record here pretty much prevents us from enforcing write-time types.

Maybe the problem is that onDidDispose isn't on the type returned. So maybe we need to change T extends IDataProvider to T extends DataProviderInfo? And then the return type can be Promise<T | undefined>?

---

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

Previously, lyonsil wrote…

fixed - also, onDidDispose being disallowed is enforced now by tsc, not just at runtime. It's hard to see exactly which previous comment you were referencing, but I think what is there now covers everything we discussed. If not, please flag it again in the latest code.

I'm talking about mentioning that createLocalObjectToProxy is particularly useful for creating network events on a network object. Before the changes in this PR, the LocalObjectToProxyCreator type had the following in its comment to indicate the reason we have this:

This is useful for setting up network events on a network object.

You are welcome to word this any way you choose if it does seem valuable to you.

---

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

 * @returns INetworkObjectDisposer wrapping the object to share
 */
const set = async <T extends object>(

Can we change this to <T extends NetworkableObject> and make objectToShare just T?

Also, I think we do not need as unknown at the bottom of this function, but I may be missing something. Does TypeScript produce an error if you remove it?

[lyonsil]

Reviewable status: 9 of 17 files reviewed, 4 unresolved discussions (waiting on @tjcouch-sil)

---

src/renderer/hooks/papi-hooks/use-data-provider.hook.ts line 91 at r6 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Please move this type assertion comment up above where the assertion is now

done

---

src/shared/models/network-object-info.model.ts line 47 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I will do that in the networkobjectService.get area :)

I think this is done

---

src/shared/services/data-provider.service.ts line 37 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Gotcha. Thanks for looking into it. Would you mind either updating the places where dataType is used elsewhere or filing a new issue to change the names please? Just want to be consistent. So in useDataProvider and useData, for example, we need to change the parameter name from dataType to providerName. (dataType is an overloaded parameter name in those two files in particular, so the actual function code shouldn't change.)

done

---

src/shared/services/data-provider.service.ts line 321 at r6 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Do we need to add Record here? Why? It is expected that extensions that define data providers will also define a data provider type that people can use to call custom methods on those data providers. I believe adding Record here pretty much prevents us from enforcing write-time types.

Maybe the problem is that onDidDispose isn't on the type returned. So maybe we need to change T extends IDataProvider to T extends DataProviderInfo? And then the return type can be Promise<T | undefined>?

done - we worked through removing Record<string, unknown> everywhere together

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

I'm talking about mentioning that createLocalObjectToProxy is particularly useful for creating network events on a network object. Before the changes in this PR, the LocalObjectToProxyCreator type had the following in its comment to indicate the reason we have this:

This is useful for setting up network events on a network object.

You are welcome to word this any way you choose if it does seem valuable to you.

done

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

Can we change this to <T extends NetworkableObject> and make objectToShare just T?

Also, I think we do not need as unknown at the bottom of this function, but I may be missing something. Does TypeScript produce an error if you remove it?

done - we worked this out together as there were complications with the Record<string, unknown> references

---

src/shared/models/disposal-interface.ts line 1 at r6 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This file name does not follow our naming conventions. Consider disposal-interface.model.ts or something. Maybe consult Ira :)

renamed

[tjcouch-sil]

Wooow 🤩🤩 so excited for these changes!! Smash that merge button!!

Reviewed 8 of 8 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lyonsil)