Create Data Provider API #122

tjcouch-sil · 2023-04-01T01:53:52Z

Created DataProviderService that exposes on the papi a way to manage data of a particular type.
Run papi.dataProvider.registerEngine('<data_type>', <your_data_provider_engine>) to register an engine object which can get, set, and manually notify of updates. set does not have to be provided (may indicate the data provider is read-only)
The data provider that is created on the papi from the data provider engine can get, set, and subscribe to changes as well as run whatever other methods the data provider engine defines
Data providers have selectors that inform the data provider of the selection of data the method call pertains to
Created useDataProvider hook to nicely wrap getting a data provider
Created useData hook to nicely wrap using a data provider in a useState-like api
Created useEventAsync hook to use events with asynchronous subscribing and unsubscribing
Created test greetings provider and "quick verse" (gets verse text from a free online api) provider as well as some renderer code and a new web view to test them
Added @extensions path alias for serving built-in extension types
Various little fixes

Resolves #59

This change is

…ta provider functionality, but papi will create a data provider to wrap that engine to provide updates

get updates instead of selector events This reverts commit c9df9cf.

…eep comparing

…s provider indicating it updated when it did not

… to useEvent

…e, minor fixes

… webview

…to send updates in other contexts beside after a successful set

…y found a situation where they are different

…ary method on data provider engine

irahopkinson

Thanks for all your work on this!

For some reason I can't try this out because I can't see the extensions. Perhaps we could pair to figure this out?

There is a bit of promise/then here. I know we only just talked about prefering async/await last week. Would it be a good idea to hit that in an immediate follow-up PR?

Reviewed 28 of 28 files at r1, all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @tjcouch-sil)

package.json line 87 at r1 (raw file):

    "electron-updater": "^5.3.0",
    "electron-window-state": "^5.0.3",
    "fast-deep-equal": "^3.1.3",

FYI: this was already a dependency of other devDependencies. We now make it production.

extensions/hello-someone/hello-someone.js line 12 at r1 (raw file):

const unsubscribers = [];

class GreetingsDataProviderEngine {

I know this is only test code but strictly speaking this doesn't need to be a class (it has no constructor or any other class specific stuff) and all the extra that comes with that in JS. This can simply be declared as an object. Hmm... maybe it does need to be a class since you are using this.people inside an arrow function.

extensions/hello-someone/hello-someone.js line 26 at r1 (raw file):

    if (selector === '*') return false;

    if (data !== this.people[selector.toLowerCase()]) {

Use nominal flow here to help with reability. You dealt with the first exception above which is good. Then check the next if (data === this.people[selector.toLowerCase()]) return false;

Then the remaining nominal code will not be indented:

this.people[selector.toLowerCase()] = data;
return true;

extensions/hello-someone/hello-someone.js line 37 at r1 (raw file):

   * @param {string} selector
   */
  get = async (selector) => {

IDataProviderEngine indicates this should not be an arrow function. Does this not apply here or is this testing that limitation?

extensions/hello-someone/hello-someone.js line 46 at r1 (raw file):

  testRandomMethod = async (things) => {
    const result = `Greetings data provider got testRandomMethod! ${things}`;
    logger.log(result);

Use logger.info to make the level obvious. Replace all logger.log( with logger.info( in all files. But do that after merging in main.

extensions/hello-someone/hello-someone.js line 61 at r1 (raw file):

  papi.webViews.addWebView({
    contentType: 'html',
    contents: `<html>

Consider importing this from an HTML file.

extensions/hello-someone/hello-someone.js line 112 at r1 (raw file):

              const name = greetingsInput.value;
              const setGreetingsInput = document.getElementById('set-greetings-input');
              greetingsDataProvider.set(name.toLowerCase(), setGreetingsInput.value).then((success) => {

style: change to something like isSuccessful

Code quote:

success

extensions/quick-verse/quick-verse.js line 16 at r1 (raw file):

   * Verses stored by the Data Provider.
   * Keys are Scripture References.
   * Values are { text: '<verse_text>', changed?: boolean }

style: isChanged

extensions/quick-verse/quick-verse.js line 57 at r1 (raw file):

    // Only heretics change Scripture, so you have to tell us you're a heretic
    if (!data.heresy) return false;

style: isHeresy, also in JSDoc above.

extensions/quick-verse/quick-verse.js line 59 at r1 (raw file):

    if (!data.heresy) return false;

    if (

Again, change this to nominal flow.

extensions/quick-verse/quick-verse.js line 94 at r1 (raw file):

        );
        const verseData = await verseResponse.json();
        const text = verseData.text.replace(/\n/g, '');

Can use replaceAll function these days rather than a regex.

Code quote:

replace(/\n/g, '')

src/renderer/hooks/papi-hooks/useData.ts line 20 at r1 (raw file):

 * @param subscriberOptions various options to adjust how the subscriber emits updates
 *
 *    WARNING: MUST BE WRAPPED IN A useState, useMemo, etc. The reference must not be updated every render

I don't understand what this warning is included for on the return? The existing example usage doesn't do any wrapping that I can see.

src/renderer/hooks/papi-hooks/useData.ts line 42 at r1 (raw file):

  // Get the data provider info for this data type
  // Note: do nothing if we received a data provider, but still run this hook. We must make sure to run the same number of hooks in all code paths)

Why must we "make sure to run the same number of hooks in all code paths"?

Code quote:

We must make sure to run the same number of hooks in all code paths

src/shared/models/DataProviderInfo.ts line 21 at r1 (raw file):

 * Returned from registering a data provider (only the process that set it up should dispose of it)
 */
// Basically a layer over DisposableDataProviderInfo

Shouldn't this be DisposableNetworkObjectInfo?

Code quote:

DisposableDataProviderInfo

src/shared/models/IDataProvider.ts line 12 at r1 (raw file):

  getDataImmediately?: boolean;
  /**
   * Whether to skip updates to the data that result in deeply equal data for this subscriber's selector

This is a little hard to understand what this option does. Can you improve the description and/or the name?

src/shared/services/DataProviderService.ts line 29 at r1 (raw file):

/** Builds the id for the data provider network object of the specified type */
const buildDataProviderObjectId = (dataType: string) =>

nit: this doesn't seem to be 'build'ing anything, how about getDataProviderObjectId?

Code quote:

buildDataProviderObjectId

src/shared/services/DataProviderService.ts line 39 at r1 (raw file):

/** Sets up the service. Only runs once and always returns the same promise after that */
const initialize = () => {

We seem to be doing this all over the place. I wonder if we can just assertNetworkReady() say in the has function below and throw if it's not. That method probably doesn't even need to be async. Other options?

src/shared/services/DataProviderService.ts line 148 at r1 (raw file):

    /** Layered set that emits an update event after running the engine's set */
    set: async (selector, data) => {
      const dpeSetResult = await dpeSet(selector, data);

style: didDpeSet?

Code quote:

dpeSetResult

src/shared/services/DataProviderService.ts line 175 at r1 (raw file):

        // but now members can't be accessed by indexing in DataProviderService
        // TODO: fix it so it is indexable but can have specific members
        // eslint-disable-next-line @typescript-eslint/no-explicit-any

I don't think this eslint disable is needed anymore. Remove it.

src/shared/services/DataProviderService.ts line 190 at r1 (raw file):

      return engineFunction;
    },
    // Type cast the data provider engine proxy because it is an IDataProvider although

Strictly speaking this is not a type cast but a type assertion. A small but important difference.

src/shared/services/DataProviderService.ts line 202 at r1 (raw file):

 * @param dataProviderEngine the object to layer over with a new data provider object
 *
 * WARNING: this function mutates the provided dataProvideEngine object. Its `forceUpdate` method is layered over.

sp: dataProviderEngine

Code quote:

dataProvideEngine

src/shared/services/NetworkObjectService.ts line 182 at r1 (raw file):

        // Took the indexing off of NetworkableObject so normal objects could be used,
        // but now members can't be accessed by indexing in NetworkObjectService
        // TODO: fix it so it is indexable but can have specific members

Would an index signature help with this?

src/shared/services/NetworkObjectService.ts line 235 at r1 (raw file):

 * so please use networkObjectContainer.networkObject to reference the network object and do not destructure it in order to preserve the reference.
 *
 * -returns the local object to proxy into a network object.

Needs a space after the dash so it gets parsed as a bullet list item.

src/shared/util/PapiUtil.ts line 1 at r1 (raw file):

import equal from 'fast-deep-equal';

Shouldn't this be 'fast-deep-equal/react'? Or are we never calling this from React or with React items?

src/shared/util/Util.ts line 41 at r1 (raw file):

 * Get a function that reduces calls to the function passed in
 * @param fn The function to debounce
 * @param delay How much delay before the most recent call to the debounced function to call the function

Specify the units, e.g. rename to msDelay or note it here in the description.

src/shared/util/Util.ts line 46 at r1 (raw file):

// We don't know the parameter types since this function can be anything
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function debounce<T extends (...args: any[]) => void>(

Could you use lodash.debounce rather than writing our own, or at least wrap it?

tjcouch-sil

Thank you for the thorough review! Let me know what you think of these things!

Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @irahopkinson)

extensions/hello-someone/hello-someone.js line 12 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I know this is only test code but strictly speaking this doesn't need to be a class (it has no constructor or any other class specific stuff) and all the extra that comes with that in JS. This can simply be declared as an object. Hmm... maybe it does need to be a class since you are using this.people inside an arrow function.

I tried changing it to an object, and it seems it didn't work with people as a member of the object. When I took people out and put it in its own object, it worked just fine. I imagine this is likely related to the weirdness between how functions are bound on classes vs as independent arrow functions. Trying to use this.people instead of moving it out of the object produced an undefined error as you suspected.

I would say it is cleaner and easier to reason about if it's a whole class instead of a number of objects. That being said, I guess people can do whatever they want as long as it works. Maybe our supported route should be classes, though, for encapsulation and such.

That being said, I will change this to an object permanently to ensure we support it if that seems valuable, though. It's nice to support the full language. Please push back if you think I should change it back to a class! But we already have a class in quick-verse.js :)

extensions/hello-someone/hello-someone.js line 26 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Use nominal flow here to help with reability. You dealt with the first exception above which is good. Then check the next if (data === this.people[selector.toLowerCase()]) return false;

Then the remaining nominal code will not be indented:
this.people[selector.toLowerCase()] = data;
return true;

Good catch. Thanks!

extensions/hello-someone/hello-someone.js line 37 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

IDataProviderEngine indicates this should not be an arrow function. Does this not apply here or is this testing that limitation?

Is there a difference in TypeScript definitions between methods and arrow functions? And yes, I made this an arrow function to test flexibility with the way people want to use the api.

I made this into an object, so I put the arrow function on get in quick-verse.js.

extensions/hello-someone/hello-someone.js line 46 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Use logger.info to make the level obvious. Replace all logger.log( with logger.info( in all files. But do that after merging in main.

Good call. Please leave this comment here so I remember to do this after merging, but I think it may be best to do all the reviewing we want before I merge and have to reformat everything.

extensions/hello-someone/hello-someone.js line 61 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Consider importing this from an HTML file.

That will come when we get a bundler :) can't import anything right now.

extensions/hello-someone/hello-someone.js line 112 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

style: change to something like isSuccessful

I'd like to consider making success a special exception because it is already what it is named in Response. It feels more concise and straight-forward than isSuccessful. Let me know your thoughts :)

extensions/quick-verse/quick-verse.js line 16 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

style: isChanged

Done.

extensions/quick-verse/quick-verse.js line 57 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

style: isHeresy, also in JSDoc above.

Done.

extensions/quick-verse/quick-verse.js line 59 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Again, change this to nominal flow.

Done.

extensions/quick-verse/quick-verse.js line 94 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Can use replaceAll function these days rather than a regex.

Done.

src/renderer/hooks/papi-hooks/useData.ts line 20 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I don't understand what this warning is included for on the return? The existing example usage doesn't do any wrapping that I can see.

I have been going back and forth about these warnings. Technically, these are all just the same as you would expect from any react prop or such meaning they cause a rerender, but they don't really naturally feel like it because they're passed into hooks.

Regardless, on this specific argument, there isn't an example of using subscriberOptions in useData. It's optional. I clarified its description to state that it must be wrapped if it is provided.

src/renderer/hooks/papi-hooks/useData.ts line 42 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Why must we "make sure to run the same number of hooks in all code paths"?

This is inherent to how React works. It keeps track of hooks between runs of functional components by index. Each hook must run every time the component is run. I just wanted to explain in here why we were doing such strange-looking stuff.

src/shared/models/DataProviderInfo.ts line 21 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Shouldn't this be DisposableNetworkObjectInfo?

Done. Thanks!

src/shared/models/IDataProvider.ts line 12 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This is a little hard to understand what this option does. Can you improve the description and/or the name?

Yeah, this one was hard to name. I negated it to receiveEqualUpdates and added more explanation. Does the name itself read better naturally? Does the explanation clear up any confusion?

Aw, I just realized these booleans don't align with the naming conventions either... :( do we really need to put "should" on the front of every one of these? Seems like this is getting kinda unwieldy. I wonder if maybe we should make an exception when booleans are in objects...? Or maybe just specifically in options?

src/shared/services/DataProviderService.ts line 39 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

We seem to be doing this all over the place. I wonder if we can just assertNetworkReady() say in the has function below and throw if it's not. That method probably doesn't even need to be async. Other options?

I am definitely interested in considering other options for doing this kind of thing, but I think we pretty much need to wait on a connection to do just about anything. I don't know if it is feasible not to wait on the NetworkService.

has reaches out to the network to figure out if any process has the data provider (implementation in NetworkObjectService.has), so it needs to be asynchronous.

src/shared/services/DataProviderService.ts line 148 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

style: didDpeSet?

I prefer to leave it as dpeSetResult because its purpose is to forward the return value from dpeSet out from its own function. A data provider's set function makes a thin layer over the engine's set, so it seems sensible in my mind to leave it as agnostic as possible. With dpeSetResult, if set changes its signature in the future, we wouldn't need to rename this variable.

I did realize I could make this implementation even more agnostic by using ...args instead of naming the parameters, so I did that.

src/shared/services/DataProviderService.ts line 175 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I don't think this eslint disable is needed anymore. Remove it.

Done.

src/shared/services/DataProviderService.ts line 190 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Strictly speaking this is not a type cast but a type assertion. A small but important difference.

Good call!

src/shared/services/DataProviderService.ts line 202 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

sp: dataProviderEngine

Done.

src/shared/services/NetworkObjectService.ts line 182 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Would an index signature help with this?

Unfortunately, index signatures demand that all members of the object align with that signature's type. As such, any object I was using as a network object had to conform to the exact signature type of NetworkableObject. Instead, I opted to take out the index signature so these objects can have whatever they want on them like other fields and such.

src/shared/services/NetworkObjectService.ts line 235 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Needs a space after the dash so it gets parsed as a bullet list item.

Done.

src/shared/util/PapiUtil.ts line 1 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Shouldn't this be 'fast-deep-equal/react'? Or are we never calling this from React or with React items?

To be honest, I don't understand the react version's use case at the moment. Maybe it's saying it allows you to compare refs to different React components...? This seems like a very specific use case that I don't think I have ever encountered. What do you think?

src/shared/util/Util.ts line 41 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Specify the units, e.g. rename to msDelay or note it here in the description.

Done.

src/shared/util/Util.ts line 46 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Could you use lodash.debounce rather than writing our own, or at least wrap it?

I used lodash at my previous company, but I kinda avoid it like the plague these days... It feels like an outdated, immensely heavy library to me at this point. You can do nearly everything in it with some other solution in vanilla JS nowadays, and I try to make tiny implementations of the few things I need otherwise. But if we want to use it, maybe we can discuss how we want to use it (importing individual functions vs using tooling to shake it out: https://www.blazemeter.com/blog/import-lodash-libraries

I would probably prefer to import individual functions if that works well with combining with the lodash used as dependencies of some of our dependencies. But I don't know if it works well or if that's wishful thinking! Thoughts? Maybe we can just make an issue on it...?

JohnThomson

I only got through the bits before useData but you may as well see what I thought so far.

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

extensions/hello-someone/hello-someone.js line 38 at r1 (raw file):

   */
  get = async (selector) => {
    if (selector === '*') return this.people;

I suppose it is guaranteed that this object will be serialized and deserialized before the client sees it, so we're not giving the client a way to trash our internal data by handing over the object?

extensions/hello-someone/hello-someone.js line 59 at r1 (raw file):

  );

  papi.webViews.addWebView({

It strikes me as bizarre that a data-provider is inserting a web view that appears to be a UI...perhaps it is just intended as a demonstration of the data it can provide? Is this something a real data-provider would do? If not at least comment.
If it's intended as a demo I think it would be a better demo to show that a separate UI component could interact with the data provider.

extensions/hello-someone/hello-someone.js line 90 at r1 (raw file):

            greetingsButton.addEventListener("click", () => {
              const greetingsInput = document.getElementById('greetings-input');
              greetingsDataProvider.get(greetingsInput.value.toLowerCase()).then((message) => {

Let's be consistent about using either 'then' or 'await' syntax.

extensions/hello-someone/hello-someone.js line 124 at r1 (raw file):

              allGreetings.innerHTML = greetingsString;
              print(greetingsString);
            });

Something to think about here: There could be thousands of people in the system, yet typically only one changes at a time. Serializing the whole list feels wasteful. (Imagine serializing all of Scripture any time some client wants to know when it changes.) I think there should be some way to request notification when any greeting changes, but get a message that just has the data about the one person whose greeting changed.
I'm not sure this is achievable without abandoning the idea that 'set' just returns true if something changed rather than explicitly raising an event.
Another aspect of this problem: the current API assumes that keys are simple, except for the '' special case. But suppose it gets just a little more complicated. Say the greetings module gets extended to store relationships as well. You can now set({"bill", friend}, "joe") which establishes that joe and bill are friends. You can get({"bill", friend}) which returns a list of people bill is friends with. We'll suppose that friendships are bidirectional, so one result of that call is that get({"joe", friend}) will now include bill.
But what if something has done a subscribe({"joe", friend})? How does the backing store know that subscriber should be notified when set({"bill", friend}, "joe") returns true?
My thinking is that as soon as the data relationships inside a provider get at all complicated, you really need to allow the set code to figure out what events need raising. I suppose there could still be a special case where the only needed notifications are to clients who subscribed to that exact key or "" but I'm not sure the simplification is worth the cost of having two mechanisms.

extensions/hello-someone/hello-someone.js line 183 at r1 (raw file):

      unsubPromises
        .map((unsubPromise) => unsubPromise.unsubscriber)
        .concat([greetingsDataProviderInfo.dispose]),

This suggests that the greetingsDataProviderInfo will be gotten rid of altogether. But might there not be other subscribers? I'm thinking we want a pair of terms different from get/dispose for "give me a connection to X"/"done using X"
Also, what happens if a subscriber neglects to do this? Or if something closes its window and it never gets a chance to?

extensions/quick-verse/quick-verse.js line 122 at r1 (raw file):

  return Promise.all(
    unsubPromises.map((unsubPromise) => unsubPromise.promise),

I don't really understand what you're doing here, but I don't see how it can be useful to run map on an array you just forced to be empty.

JohnThomson

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

src/renderer/hooks/papi-hooks/useData.ts line 56 at r1 (raw file):

  // Make const variables of the data provider so TypeScript knows they won't change
  const dataProvider = dataProviderTemp;
  const isDisposed = isDisposedTemp;

I think you can just do const [dataProvider, isDisposed] = dataType.

src/shared/services/DataProviderService.ts line 106 at r1 (raw file):

    const unsubscribe = onDidUpdate(callbackWithUpdate);

    if (getDataImmediately) {

If getDataImmediately is false, looks like dataPrevious will not be initialized until the first callbackWithUpdate. I'm surprised the compiler doesn't have a fit about that. But is it actually a problem? If it's actually possible to get a callback when the data hasn't (deeply) changed, could a random initial value make it seem that something has changed when it hasn't? Or that it hasn't when it really has? Probably more likely if, say, TGetValue can legitimately be undefined or a number.

src/shared/services/DataProviderService.ts line 108 at r1 (raw file):

    if (getDataImmediately) {
      // Get the data to run the callback immediately so it has the data
      const data = await dataProviderContainer.networkObject.get(selector);

shouldn't we set dataPrevious here?

src/shared/services/DataProviderService.ts line 153 at r1 (raw file):

    },
    /** Layered get that runs the engine's get */
    get: dataProviderEngine.get.bind(dataProviderEngine),

Feels like, with a little finagling here, you could use the result from the dataProviderEngine to set the dataPrevious of the subscriber. Then,

If the client has read the data at all, you always have a dataPrevious. getDataImmediately can possibly go away
You don't need two calls to get when getDataImmediately is true.

src/shared/services/DataProviderService.ts line 224 at r1 (raw file):

    throw new Error('Data provider engine does not have a get function');
  if (!dataProviderEngine.set || typeof dataProviderEngine.set !== 'function')
    throw new Error('Data provider engine does not have a set function');

Sure it's not valid to have a read-only data provider? E.g. for a reference Scripture text.

…view cleanup

tjcouch-sil

Reviewable status: 16 of 28 files reviewed, 32 unresolved discussions (waiting on @irahopkinson and @JohnThomson)

extensions/hello-someone/hello-someone.js line 38 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I suppose it is guaranteed that this object will be serialized and deserialized before the client sees it, so we're not giving the client a way to trash our internal data by handing over the object?

Actually, if another extension main file (code run from the extension host) runs this, it will receive the full object itself! I suppose maybe it is the extension devs' responsibility to write their extensions so other extensions can't access their objects in ways they don't want. I will shallow clone people and make a note here; should we do anything else about it? I suppose we could alternatively model returning a proxy whose set doesn't work.

extensions/hello-someone/hello-someone.js line 59 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It strikes me as bizarre that a data-provider is inserting a web view that appears to be a UI...perhaps it is just intended as a demonstration of the data it can provide? Is this something a real data-provider would do? If not at least comment.
If it's intended as a demo I think it would be a better demo to show that a separate UI component could interact with the data provider.

Do you consider it to be a problem that an extension defines both a data provider and a web view that ingests that provider? I think it might make sense to separate them in many cases, but I don't think it is quite necessary that they would have to be separated.

Side note: this PR does demonstrate an extension (quick-verse.js) providing data and another extension (hello-world.js and the renderer itself). :)

extensions/hello-someone/hello-someone.js line 90 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Let's be consistent about using either 'then' or 'await' syntax.

Done.

extensions/hello-someone/hello-someone.js line 124 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Something to think about here: There could be thousands of people in the system, yet typically only one changes at a time. Serializing the whole list feels wasteful. (Imagine serializing all of Scripture any time some client wants to know when it changes.) I think there should be some way to request notification when any greeting changes, but get a message that just has the data about the one person whose greeting changed.
I'm not sure this is achievable without abandoning the idea that 'set' just returns true if something changed rather than explicitly raising an event.
Another aspect of this problem: the current API assumes that keys are simple, except for the '' special case. But suppose it gets just a little more complicated. Say the greetings module gets extended to store relationships as well. You can now set({"bill", friend}, "joe") which establishes that joe and bill are friends. You can get({"bill", friend}) which returns a list of people bill is friends with. We'll suppose that friendships are bidirectional, so one result of that call is that get({"joe", friend}) will now include bill.
But what if something has done a subscribe({"joe", friend})? How does the backing store know that subscriber should be notified when set({"bill", friend}, "joe") returns true?
My thinking is that as soon as the data relationships inside a provider get at all complicated, you really need to allow the set code to figure out what events need raising. I suppose there could still be a special case where the only needed notifications are to clients who subscribed to that exact key or "" but I'm not sure the simplification is worth the cost of having two mechanisms.

Regarding "I think there should be some way to request notification when any greeting changes, but get a message that just has the data about the one person whose greeting changed.", you can provide the subscribe option receiveEqualUpdates, and your callback will be run on any updates even if your data didn't change :)

I don't understand the complex relationship example, so I would love to hear more about this! Part of me wonders if the selector events concept I have been considering would solve the issues about different kinds of events going out. Part of me wonders if we need something deeper. Would be good to brainstorm about your concerns together!

extensions/hello-someone/hello-someone.js line 183 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

This suggests that the greetingsDataProviderInfo will be gotten rid of altogether. But might there not be other subscribers? I'm thinking we want a pair of terms different from get/dispose for "give me a connection to X"/"done using X"
Also, what happens if a subscriber neglects to do this? Or if something closes its window and it never gets a chance to?

Actually, greetingsDataProviderInfo.dispose disposes of the data provider itself and sends events out to subscribers on greetingsDataProviderInfo.onDidDispose. Another good reason to get rid of the whole Info pattern and just modify the object! :) Made issue #125

extensions/quick-verse/quick-verse.js line 122 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I don't really understand what you're doing here, but I don't see how it can be useful to run map on an array you just forced to be empty.

This is boilerplate I pulled from another extension file. Honestly, the return from activate on extensions needs to be reworked completely. #126, #50, #54

src/renderer/hooks/papi-hooks/useData.ts line 56 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I think you can just do const [dataProvider, isDisposed] = dataType.

This is a tricky situation. We want to call the useDataProvider hook to get dataProvider and isDisposed if dataType is a string, but we just want to get the dataProvider and isDisposed from dataType if it is already a useDataProvider. However, we always have to call the same number of hooks no matter what code path we follow, so we have to make dataProvider and isDisposed able to be changed based on dataType. But then if we leave them to be changeable, TypeScript gets upset that they might be undefined in the arrow functions below because TypeScript thinks they can be undefined, and they are no longer in the same context so their type narrowing that we did by defining them in both the if and the else depending on what dataType is goes away. But we know that no other code has touched those variables, so they aren't undefined. So my solution was to make temporary let variables for them then make them consts when they were both defined.

I did think of a tweak to make this a little less weird and be less dependent on the implementation of the useDataProvider hook, so I'll do that now.

src/shared/services/DataProviderService.ts line 106 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

If getDataImmediately is false, looks like dataPrevious will not be initialized until the first callbackWithUpdate. I'm surprised the compiler doesn't have a fit about that. But is it actually a problem? If it's actually possible to get a callback when the data hasn't (deeply) changed, could a random initial value make it seem that something has changed when it hasn't? Or that it hasn't when it really has? Probably more likely if, say, TGetValue can legitimately be undefined or a number.

I think you're right that starting dataPrevious as undefined is a bug. If we had getDataImmediately set to false, then we got an update whose data was undefined, it would not run the callback. Good catch. Thanks!

src/shared/services/DataProviderService.ts line 108 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

shouldn't we set dataPrevious here?

Yes, we should! Thanks for the good catch.

src/shared/services/DataProviderService.ts line 153 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Feels like, with a little finagling here, you could use the result from the dataProviderEngine to set the dataPrevious of the subscriber. Then,

If the client has read the data at all, you always have a dataPrevious. getDataImmediately can possibly go away

You don't need two calls to get when getDataImmediately is true.

Are you essentially suggesting I go ahead and put in a cache that holds responses for different selectors? I suppose I could do that, but I think it might be best to wait on that. It seems a bit complex to figure out exactly how to bust the cache. I will put a TODO in.

src/shared/services/DataProviderService.ts line 224 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Sure it's not valid to have a read-only data provider? E.g. for a reference Scripture text.

I figured we could support making a read-only data provider by having the set be defined but throw an exception on use. I suppose I could refactor, but I think I'll just throw a TODO: in there for now if that seems reasonable :)

irahopkinson

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

extensions/hello-someone/hello-someone.js line 12 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I tried changing it to an object, and it seems it didn't work with people as a member of the object. When I took people out and put it in its own object, it worked just fine. I imagine this is likely related to the weirdness between how functions are bound on classes vs as independent arrow functions. Trying to use this.people instead of moving it out of the object produced an undefined error as you suspected.

I would say it is cleaner and easier to reason about if it's a whole class instead of a number of objects. That being said, I guess people can do whatever they want as long as it works. Maybe our supported route should be classes, though, for encapsulation and such.

That being said, I will change this to an object permanently to ensure we support it if that seems valuable, though. It's nice to support the full language. Please push back if you think I should change it back to a class! But we already have a class in quick-verse.js :)

Nice job getting that to work. Until we have a good reason not to, I think it's good to support both objects and classes.

extensions/hello-someone/hello-someone.js line 37 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Is there a difference in TypeScript definitions between methods and arrow functions? And yes, I made this an arrow function to test flexibility with the way people want to use the api.

I made this into an object, so I put the arrow function on get in quick-verse.js.

Yes there is a difference. I'm fine with it being an arrow function for testing.

extensions/hello-someone/hello-someone.js line 112 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I'd like to consider making success a special exception because it is already what it is named in Response. It feels more concise and straight-forward than isSuccessful. Let me know your thoughts :)

Err... we created that type so it's easy for us to change it to follow our guidelines in that type declaration. However here, I'm particularly interested in making the tenary operator more readable 2 lines below. It's made difficult by its inline use. Can you do something about that?

extensions/hello-someone/hello-someone.js line 22 at r2 (raw file):

   * @param {string} data
   */
  set: async (selector, data) => {

This wasn't an arrow function before and you can have async methods in objects. It isn't obvious to me that this needs to be an arrow function (nor the 2 below either). From looking through the code briefly it looks like the method is always called off the object and not passed around anywhere. Also they are no longer using this either so I think they can all be object methods. Is that right?

It's fine if this is for test purposes. But having at least one arrow function and one object method might test more. What do you think?

src/shared/models/IDataProvider.ts line 12 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Yeah, this one was hard to name. I negated it to receiveEqualUpdates and added more explanation. Does the name itself read better naturally? Does the explanation clear up any confusion?

Aw, I just realized these booleans don't align with the naming conventions either... :( do we really need to put "should" on the front of every one of these? Seems like this is getting kinda unwieldy. I wonder if maybe we should make an exception when booleans are in objects...? Or maybe just specifically in options?

Thanks for the rename and updated description. Good improvement. However I think the word Equal is what makes it difficult. What about receiveAllUpdates? It might also help to specify what happens when it's false - will I receive updates only based on my selector or none at all?

In terms of name guidelines, adding will or can would suffice, i.e. willGetDataImmediately and willReceiveAllUpdates.

src/shared/services/DataProviderService.ts line 39 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I am definitely interested in considering other options for doing this kind of thing, but I think we pretty much need to wait on a connection to do just about anything. I don't know if it is feasible not to wait on the NetworkService.

has reaches out to the network to figure out if any process has the data provider (implementation in NetworkObjectService.has), so it needs to be asynchronous.

If papi starts by awaiting to initialize the network, can't everything after that assume it's ready?

src/shared/services/DataProviderService.ts line 148 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I prefer to leave it as dpeSetResult because its purpose is to forward the return value from dpeSet out from its own function. A data provider's set function makes a thin layer over the engine's set, so it seems sensible in my mind to leave it as agnostic as possible. With dpeSetResult, if set changes its signature in the future, we wouldn't need to rename this variable.

I did realize I could make this implementation even more agnostic by using ...args instead of naming the parameters, so I did that.

Sounds reasonable and now the whole thing looks more generic too - good spotting on the args to help with that.

src/shared/services/DataProviderService.ts line 85 at r2 (raw file):

    /** The most recent data before the newest update. Used for deep comparison checks to prevent useless updates */
    // Start this out as a new object so updates definitely run the callback (including if the data is undefined)

This is a good improvement to start with a defined object. We should always aim to do that in general otherwise our code is littered with checks for undefined. In which case this comment seems unnecessary but perhaps that is not our default in general?

src/shared/util/PapiUtil.ts line 1 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

To be honest, I don't understand the react version's use case at the moment. Maybe it's saying it allows you to compare refs to different React components...? This seems like a very specific use case that I don't think I have ever encountered. What do you think?

I didn't understand it either. Perhaps add a comment that there is a React version and we might need to use it?

src/shared/util/Util.ts line 46 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I used lodash at my previous company, but I kinda avoid it like the plague these days... It feels like an outdated, immensely heavy library to me at this point. You can do nearly everything in it with some other solution in vanilla JS nowadays, and I try to make tiny implementations of the few things I need otherwise. But if we want to use it, maybe we can discuss how we want to use it (importing individual functions vs using tooling to shake it out: https://www.blazemeter.com/blog/import-lodash-libraries

I would probably prefer to import individual functions if that works well with combining with the lodash used as dependencies of some of our dependencies. But I don't know if it works well or if that's wishful thinking! Thoughts? Maybe we can just make an issue on it...?

An issue sounds good, go ahead and make one.

tjcouch-sil

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

extensions/hello-someone/hello-someone.js line 37 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Yes there is a difference. I'm fine with it being an arrow function for testing.

Ah interesting. After a good bit of google searching, I found the difference from here:

interface myInterface {
    foo1(args: string): void;
    foo2: (args: string) => void;
}

And an example object function:

const person = {
  firstName: "John",
  lastName: "Doe",
  id: 5566,
  fullName() {
    return this.firstName + " " + this.lastName;
  }
};

I have done some updating to add more variations and such. Learned a lot here. Thanks!

extensions/hello-someone/hello-someone.js line 22 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This wasn't an arrow function before and you can have async methods in objects. It isn't obvious to me that this needs to be an arrow function (nor the 2 below either). From looking through the code briefly it looks like the method is always called off the object and not passed around anywhere. Also they are no longer using this either so I think they can all be object methods. Is that right?

It's fine if this is for test purposes. But having at least one arrow function and one object method might test more. What do you think?

Wow, I didn't realize you could attach functions to objects like this! Learned lots from this! See below for my comments about it.

src/shared/services/DataProviderService.ts line 39 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

If papi starts by awaiting to initialize the network, can't everything after that assume it's ready?

It could, but papi is an exported object that is always synchronously defined; it doesn't wait for the network to initialize. Each service on it must be ready on its own. I think putting initialize conditions individually into each service also helps with initialization order (if one service depends on another, it can await the other service here). We also do not use papi in our core code generally but rather get references to the services directly because of potential dependency cycle issues.

src/shared/services/DataProviderService.ts line 85 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This is a good improvement to start with a defined object. We should always aim to do that in general otherwise our code is littered with checks for undefined. In which case this comment seems unnecessary but perhaps that is not our default in general?

Indeed that is not our default. We discussed in the code style conversation today that it is valid for an object to be undefined if it isn't populated until a later time. This is just a very special case where undefined is an update that we would need to give to the callback.

src/shared/util/Util.ts line 46 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

An issue sounds good, go ahead and make one.

#134 thanks!

JohnThomson

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

extensions/hello-someone/hello-someone.js line 59 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Do you consider it to be a problem that an extension defines both a data provider and a web view that ingests that provider? I think it might make sense to separate them in many cases, but I don't think it is quite necessary that they would have to be separated.

Side note: this PR does demonstrate an extension (quick-verse.js) providing data and another extension (hello-world.js and the renderer itself). :)

It makes some sense that a DP that is just a demo would have a client-type UI as well. I think it partly surprised me because I had the impression that different degrees and possibly mechanisms of sandboxing were involved. I think I also had a vague idea that data providers might run in Node whereas UI has to run in an actual browser (or embedded browser control). If none of those are issues, all that is left is whether combining them makes a good demo. As long you're sure the UI demo isn't depending on capabilities that are only available because it's in the same code chunk, I don't object if you want to keep them together.

extensions/hello-someone/hello-someone.js line 124 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think maybe the following issues address these concerns as we discussed: #109, #135, #136. I don't think I fully understand the use case for sending events yet, so I think it would be good to discuss that more sometime (maybe for another issue?).

I think we talked about this. The idea that a subscriber gets notified of ALL changes and re-submits the query to see whether anything changed is certainly sufficient to ensure the UI updates as needed, but I'm a little worried about performance if the client doesn't ever get any help to know whether something it cares about changed. But it probably makes sense to start with this approach and later, if we need some optimization to avoid resubmitting too many queries on things that didn't change, we can consider it then.

src/renderer/hooks/papi-hooks/useData.ts line 56 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This is a tricky situation. We want to call the useDataProvider hook to get dataProvider and isDisposed if dataType is a string, but we just want to get the dataProvider and isDisposed from dataType if it is already a useDataProvider. However, we always have to call the same number of hooks no matter what code path we follow, so we have to make dataProvider and isDisposed able to be changed based on dataType. But then if we leave them to be changeable, TypeScript gets upset that they might be undefined in the arrow functions below because TypeScript thinks they can be undefined, and they are no longer in the same context so their type narrowing that we did by defining them in both the if and the else depending on what dataType is goes away. But we know that no other code has touched those variables, so they aren't undefined. So my solution was to make temporary let variables for them then make them consts when they were both defined.

I did think of a tweak to make this a little less weird and be less dependent on the implementation of the useDataProvider hook, so I'll do that now.

I think I got confused by the names. dataType suggests something like an object class more than something that is either a data provider or an ID for one. How about calling it dataProviderSource, then the special case is something like gotDataProviderById = isString(dataProvider).
But also consider pushing the logic down into useDataProvider. What if THAT had a special case for an argument that is already [data provider, isDisposed], and just returned it immediately? It would certainly simplify this code, if it didn't complicate the code there too much.
BTW why do you pass around a separate indication of whether the DP is disposed? Is it not possible to ask the DP itself rather than maintaining a separate record that could get forgotten or out of date? If it's so thoroughly disposed of that you can't invoke one of its methods to check whether it is disposed, it might at least be worth a class to wrap the DP itself and its disposed state. But either way there is a concern: how does everyone who cares find out that a DP has been disposed?

src/shared/services/DataProviderService.ts line 153 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Are you essentially suggesting I go ahead and put in a cache that holds responses for different selectors? I suppose I could do that, but I think it might be best to wait on that. It seems a bit complex to figure out exactly how to bust the cache. I will put a TODO in.

No, that wasn't my point.
I'm imagining a client doing something like this:
const [greeting, setGreeting] = useState("");
useEffect(async () =>
dp.subscribe("greeting", (newGreeting) => setGreeting(newGreeting);
const result = await (dp.get("greeting");
setGreeting(result);
}, []);

{greeting}

I probably don't have that exactly right, but if it's close, the sad thing is that the client code and the subscription initialization code are both having to do the same get...the client to get the initial value, and the subscribe code to establish dataPrevious.
What if subscribe returned the same result as get? Then the get that the subscribe code does to establish dataPrevious would also serve to give the client the initial value...which BTW also helps ensure that there are no possible race conditions between getting the initial value and setting up the subscription.
Another variation would be that subscribe always invokes the callback with the initial value it gets for dataPrevious, so the subscriber doesn't have to do anything but subscribe. (I think that might be what useData already does? If so, it may be helpful to provide some clarifying comments: when would a client just do a get, and would it ever make sense to do a get at all if there is a need to subscribe?)
Or maybe I'm totally confused. This PR is too big to keep entirely in my head.

… made useDataProvider always return dataProvider once it has been retrieved instead of not returning when disposed, explained get vs subscribe more

tjcouch-sil

Reviewable status: 23 of 33 files reviewed, 8 unresolved discussions (waiting on @irahopkinson, @JohnThomson, and @lyonsil)

extensions/hello-someone/hello-someone.js line 59 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It makes some sense that a DP that is just a demo would have a client-type UI as well. I think it partly surprised me because I had the impression that different degrees and possibly mechanisms of sandboxing were involved. I think I also had a vague idea that data providers might run in Node whereas UI has to run in an actual browser (or embedded browser control). If none of those are issues, all that is left is whether combining them makes a good demo. As long you're sure the UI demo isn't depending on capabilities that are only available because it's in the same code chunk, I don't object if you want to keep them together.

This code is tricky to read. Essentially, the node extension has to provide an HTML page that the renderer then displays in an iframe. So this HTML page here will be sent to the frontend and will run there with some sandboxing. This demonstrates that providers and UI can be run from different places because the UI code here could theoretically be provided by any extension and is not tied down to this node code.

Hopefully, when I implement packing on extensions, it will be much easier to tell what's going on as the HTML page will be able to be in a different file and just pulled in here instead of being all in the same file.

extensions/hello-world/hello-world.js line 198 at r6 (raw file):

Previously, lyonsil wrote…

Is this a case where logger.log should be converted to logger.info? In general in other languages I always try to use logger.<level>("message"), like logger.debug, 'logger.info, 'logger.warn, etc. I see several other cases where logger.log is used in other files, too.

Yep! I plan to fix all of these when I merge in from main. Ira has a comment on here somewhere that I am leaving open to remind myself to do a search in files for logger.log :)

src/renderer/hooks/papi-hooks/useData.ts line 56 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I think I got confused by the names. dataType suggests something like an object class more than something that is either a data provider or an ID for one. How about calling it dataProviderSource, then the special case is something like gotDataProviderById = isString(dataProvider).
But also consider pushing the logic down into useDataProvider. What if THAT had a special case for an argument that is already [data provider, isDisposed], and just returned it immediately? It would certainly simplify this code, if it didn't complicate the code there too much.
BTW why do you pass around a separate indication of whether the DP is disposed? Is it not possible to ask the DP itself rather than maintaining a separate record that could get forgotten or out of date? If it's so thoroughly disposed of that you can't invoke one of its methods to check whether it is disposed, it might at least be worth a class to wrap the DP itself and its disposed state. But either way there is a concern: how does everyone who cares find out that a DP has been disposed?

I refactored so useDataProvider contains the complexity of accepting [data provider, is Disposed]. It is somewhat cleaner! We can't just return from useDataProvider if what's passed in is the return of that hook because that would change the number of hooks we're running depending on the code path.

Right now, one reason for why isDisposed is a separate item is that, otherwise, each developer would have to implement their own useState and useEvent when they want to listen to the data provider disposal. The isDisposed variable returned is "reactive" without the user having to do anything to make it so. It is arguable we could just split that part out into a separate hook!

But the bigger issue is that we are currently returning DataProviderInfo-type objects that contain the data provider and an accompanying dispose event from the data provider service. People suggested we not mutate network objects (like data providers) passed into the papi originally, but I think I have used this Info api enough at this point that I'm ready to simplify it to returning the (mutated) data provider itself instead of its "info", which is very unintuitive and cumbersome in most use cases so far. As such, there is an issue for removing the Info api pattern #125

src/shared/services/DataProviderService.ts line 153 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

No, that wasn't my point.
I'm imagining a client doing something like this:
const [greeting, setGreeting] = useState("");
useEffect(async () =>
dp.subscribe("greeting", (newGreeting) => setGreeting(newGreeting);
const result = await (dp.get("greeting");
setGreeting(result);
}, []);

{greeting}

I probably don't have that exactly right, but if it's close, the sad thing is that the client code and the subscription initialization code are both having to do the same get...the client to get the initial value, and the subscribe code to establish dataPrevious.
What if subscribe returned the same result as get? Then the get that the subscribe code does to establish dataPrevious would also serve to give the client the initial value...which BTW also helps ensure that there are no possible race conditions between getting the initial value and setting up the subscription.
Another variation would be that subscribe always invokes the callback with the initial value it gets for dataPrevious, so the subscriber doesn't have to do anything but subscribe. (I think that might be what useData already does? If so, it may be helpful to provide some clarifying comments: when would a client just do a get, and would it ever make sense to do a get at all if there is a need to subscribe?)
Or maybe I'm totally confused. This PR is too big to keep entirely in my head.

Ah I see. Thank you for clarifying. Yes, this is a big and confusing PR! I appreciate your patience and determination :)

createDataProviderSubscriber creates a subscriber function that uses retrieveDataImmediately (previously named getDataImmediately, but that was deemed confusing to be a boolean and not a function), provided by the user who is subscribing, to do what you are suggesting already: it runs a get immediately when you subscribe and runs your callback with the result so people don't have to subscribe and then run a get as well. It also handles the race condition between a subscription update coming out and the get returning so only the latest data is run.

Essentially, retrieveDataImmediately (defaults to true) tells the subscribe function to run the callback you are passing in as soon as possible with the relevant data from the provider. Internally, we handle the race between the update event going out and our initial get trying to get the latest data in case an update comes out while you're running get.

We are providing the useData hook, which gives the most up-to-date data at any given time (unless you manually specify retrieveDataImmediately is false). Under the hood, it runs a subscriber, handles the complexity of reactive async code in React, and handles unsubscribing for you.

We have an api pattern of returning the unsubscriber function when you run a subscribe function, so I think returning the data you get like { unsubscriber, data } might be a bit more confusing than just running the callback, especially if you do not intend to get the data right away.

I added a number of comments here and in IDataProvider.ts to clarify functionality.

irahopkinson

Reviewed 4 of 5 files at r6, 3 of 5 files at r7, 3 of 3 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @JohnThomson, @lyonsil, and @tjcouch-sil)

tsconfig.json line 15 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I'm not sure I'm following. The extensions folder should theoretically never be bundled in with any process bundles; I'm just using the folder for types. Do you want me to add exclusion rules for them?

All good, thanks for the clarification.

src/renderer/hooks/papi-hooks/useDataProvider.ts line 31 at r8 (raw file):

// eslint-disable-next-line @typescript-eslint/no-explicit-any
function useDataProvider<T extends IDataProvider<any, any, any>>(
  dataProviderSource: string | [T | undefined, boolean] | undefined,

This appears to be duplicated below?

Code quote:

function useDataProvider<T extends IDataProvider<any, any, any>>(
  dataProviderSource: string | [T | undefined, boolean] | undefined,
): [T | undefined, boolean];

src/renderer/testing/TestQuickVerseHeresyPanel.tsx line 1 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

It could be, but I left this in to demonstrate the difference between using useDataProvider primarily and useData primarily. I also left it in because it demonstrates that we can get updates to data changes between panels :)

Sounds good.

tjcouch-sil

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

src/renderer/hooks/papi-hooks/useDataProvider.ts line 31 at r8 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This appears to be duplicated below?

Indeed, it is unfortunate that this has to be the case, but only the declared overload signatures are usable (as opposed to the implementation signature). I want to be able to pass in a variable that is either a string or the return from this hook, so I had to make an overload signature that is the same as the implementation signature. Thanks for checking!

JohnThomson

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

src/renderer/hooks/papi-hooks/useData.ts line 56 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I refactored so useDataProvider contains the complexity of accepting [data provider, is Disposed]. It is somewhat cleaner! We can't just return from useDataProvider if what's passed in is the return of that hook because that would change the number of hooks we're running depending on the code path.

Right now, one reason for why isDisposed is a separate item is that, otherwise, each developer would have to implement their own useState and useEvent when they want to listen to the data provider disposal. The isDisposed variable returned is "reactive" without the user having to do anything to make it so. It is arguable we could just split that part out into a separate hook!

But the bigger issue is that we are currently returning DataProviderInfo-type objects that contain the data provider and an accompanying dispose event from the data provider service. People suggested we not mutate network objects (like data providers) passed into the papi originally, but I think I have used this Info api enough at this point that I'm ready to simplify it to returning the (mutated) data provider itself instead of its "info", which is very unintuitive and cumbersome in most use cases so far. As such, there is an issue for removing the Info api pattern #125

See my comment on useDataProvider. My intuition is that most clients will (like this one) behave the same when dataProvider is undefined and when isDisposed is true. (I'm guessing you may as well return dataProvider as undefined if isDisposed is true...it is bound to be a mistake to use a disposed DP, so may as well fail fast.) So isDisposed only serves to distinguish whether dataProvider is undefined because you haven't gotten it yet or because it was later disposed. (You can still re-render on dispose, setting DP back to undefined.)
There may be a few clients that care whether DP is being loaded or has been disposed, but unless that is very common I'd think it better to let those clients handle the complexity of remembering that they once got a DP rather than make all clients deal with the complexity of having useDataProvider returning disposed information.
Maybe nearly all clients do care...for example, want to show a "Loading..." message during init but not after disposal. Then my question would be, why dispose of the DP while the UI is still active at all? Sorry, feels like I'm second-guessing everything you've done...but complexity is your enemy, this is your prime chance to resist any of it you don't absolutely need.

src/renderer/hooks/papi-hooks/useDataProvider.ts line 31 at r8 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Indeed, it is unfortunate that this has to be the case, but only the declared overload signatures are usable (as opposed to the implementation signature). I want to be able to pass in a variable that is either a string or the return from this hook, so I had to make an overload signature that is the same as the implementation signature. Thanks for checking!

All the overloads return [T | undefined, boolean] and take a single argument. IIRC, the article you sent me recommends using a composite argument type rather than overloads in this case.

src/renderer/hooks/papi-hooks/useDataProvider.ts line 36 at r8 (raw file):

function useDataProvider<T extends IDataProvider<any, any, any>>(
  dataProviderSource: string | [T | undefined, boolean] | undefined,
): [T | undefined, boolean] {

Now that I see the logic together in one place, it feels weird. A client that wants, say, the translation notes data provider seems very unlikely to want a disposed object that it can't use. I presume, though, that the expectation is that this hook will render at least three times:

initial render yields [undefined, false]; client will render something indicating that it is loading
after creating the DP, renders [dp, false]; client will render something based on further async calls to the DP
at some point, the DP gets disposed; now it re-renders with [undefined, true] and the client presumably renders something indicating that the data is, for some reason, no longer available?
Is it really important to distinguish the situations where the DP is not yet instantiated from when it is disposed? Seems like you would try not to dispose it while the UI is still using it. In which case, maybe you could just return T|undefined, and re-render with the DP undefined if the client is still connected when the DP is disposed? If a client really needs to know the difference, it can make itself a state that tells whether it has ever received a valid DP.
I'm also not really following the use case for passing in a DP rather than a string. Are you thinking of a component that might be either a child or a root of some UI? Trying to make only one thing in a hierarchy be responsible for creating the DP? My instincts are saying you might rather want to use the React useContext hook to get a DP that is put into a context by the appropriate outer component using createContext.
Sorry, I may be misunderstanding totally and confusing things. I don't have the whole picture yet.

…ons as the check for if one already exists with this id

tjcouch-sil

Reviewable status: 28 of 33 files reviewed, 5 unresolved discussions (waiting on @irahopkinson, @JohnThomson, and @lyonsil)

src/renderer/hooks/papi-hooks/useData.ts line 56 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

See my comment on useDataProvider. My intuition is that most clients will (like this one) behave the same when dataProvider is undefined and when isDisposed is true. (I'm guessing you may as well return dataProvider as undefined if isDisposed is true...it is bound to be a mistake to use a disposed DP, so may as well fail fast.) So isDisposed only serves to distinguish whether dataProvider is undefined because you haven't gotten it yet or because it was later disposed. (You can still re-render on dispose, setting DP back to undefined.)
There may be a few clients that care whether DP is being loaded or has been disposed, but unless that is very common I'd think it better to let those clients handle the complexity of remembering that they once got a DP rather than make all clients deal with the complexity of having useDataProvider returning disposed information.
Maybe nearly all clients do care...for example, want to show a "Loading..." message during init but not after disposal. Then my question would be, why dispose of the DP while the UI is still active at all? Sorry, feels like I'm second-guessing everything you've done...but complexity is your enemy, this is your prime chance to resist any of it you don't absolutely need.

You have good points. I will refactor useDataProvider to stop providing isDisposed.

src/renderer/hooks/papi-hooks/useDataProvider.ts line 31 at r8 (raw file):

Previously, JohnThomson (John Thomson) wrote…

All the overloads return [T | undefined, boolean] and take a single argument. IIRC, the article you sent me recommends using a composite argument type rather than overloads in this case.

Indeed, it does recommend doing so. However, I consider its recommendation to be a slightly different use case than ours. The reason I listed these different overloads is that I wanted the string version of the argument to have a different name, dataType, so it felt more natural. It seems likely to me that this hook will be used with a string as the argument in almost all use cases outside of internal hooks that layer over this one. As such, I thought it best to add overloads that make passing in a string feel natural because dataType is the name of this string in DataProviderService's functions as well. I certainly understand a desire to simplify, though.

I imagine a common usage of this hook would probably look like this:

Its hint shows dataType as the parameter name in the function signature. It does however, still call the parameter dataProviderSource in the JSDoc:

I lean a bit toward having the overloads so the string argument can be called dataType, but I'm not particularly hard toward that side. Let me know your thoughts. Thanks!

src/renderer/hooks/papi-hooks/useDataProvider.ts line 36 at r8 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Now that I see the logic together in one place, it feels weird. A client that wants, say, the translation notes data provider seems very unlikely to want a disposed object that it can't use. I presume, though, that the expectation is that this hook will render at least three times:

initial render yields [undefined, false]; client will render something indicating that it is loading

after creating the DP, renders [dp, false]; client will render something based on further async calls to the DP

at some point, the DP gets disposed; now it re-renders with [undefined, true] and the client presumably renders something indicating that the data is, for some reason, no longer available?
Is it really important to distinguish the situations where the DP is not yet instantiated from when it is disposed? Seems like you would try not to dispose it while the UI is still using it. In which case, maybe you could just return T|undefined, and re-render with the DP undefined if the client is still connected when the DP is disposed? If a client really needs to know the difference, it can make itself a state that tells whether it has ever received a valid DP.
I'm also not really following the use case for passing in a DP rather than a string. Are you thinking of a component that might be either a child or a root of some UI? Trying to make only one thing in a hierarchy be responsible for creating the DP? My instincts are saying you might rather want to use the React useContext hook to get a DP that is put into a context by the appropriate outer component using createContext.
Sorry, I may be misunderstanding totally and confusing things. I don't have the whole picture yet.

You have good points. I will refactor useDataProvider to stop providing isDisposed.

Regarding passing in a string or a DP, I'm trying to provide an easy way to avoid having one component getting the data provider multiple times. See useDataProvider and useData in TestQuickVerseHeresypanel.tsx for an example.

JohnThomson

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

src/renderer/hooks/papi-hooks/useData.ts line 11 at r9 (raw file):

/**
 * Subscribes to run a callback on a data provider's data with specified selector
 * @param dataProviderSource string data type to get data provider for OR dataProvider (result of useDataProvider if you

Consider spelling out
@param dataProviderSource (first overload) ...
@param dataProvider (second overload)...

If the compiler/doc generator/whatever is using these @param things will let you.

src/renderer/hooks/papi-hooks/useDataProvider.ts line 36 at r8 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

You have good points. I will refactor useDataProvider to stop providing isDisposed.

Regarding passing in a string or a DP, I'm trying to provide an easy way to avoid having one component getting the data provider multiple times. See useDataProvider and useData in TestQuickVerseHeresypanel.tsx for an example.

Hmm, I wonder what dataProviderSource will do if asked repeatedly for the same DP. Is it going to make another instance, or quickly look up the same one? If it will efficiently provide the same one, ideally without another render, I would wonder whether the overload that takes the existing DP was premature optimization.
Still it's done now and the result is not bad, except perhaps in the couple of places where we have the three definitions of the function.
I think I would be inclined to make it look MORE like two overloads and an implementation. Something like,

Get the specified data provider (returns undefined in the initial render while it is being created, and again in an additional render after it has been disposed)
...first overload...

Return the same Data Provider passed in (this overload makes it easier for some clients to follow the rules of hooks)
...second overload...

Implementation of the two overloads:
...the implementation...

But it's also OK as is...this is just an idea you can use if you think it helps. (The approach has some appeal for useData, too, but there it would be harder to do it without repeating a lot.)

irahopkinson

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

…t try to create multiple

…k object like we were before

tjcouch-sil

Reviewable status: 22 of 34 files reviewed, 3 unresolved discussions (waiting on @irahopkinson, @JohnThomson, @lyonsil, and @param)

src/renderer/hooks/papi-hooks/useData.ts line 11 at r9 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Consider spelling out
@param dataProviderSource (first overload) ...
@param dataProvider (second overload)...

If the compiler/doc generator/whatever is using these @param things will let you.

Done. Good idea.

src/renderer/hooks/papi-hooks/useDataProvider.ts line 36 at r8 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Hmm, I wonder what dataProviderSource will do if asked repeatedly for the same DP. Is it going to make another instance, or quickly look up the same one? If it will efficiently provide the same one, ideally without another render, I would wonder whether the overload that takes the existing DP was premature optimization.
Still it's done now and the result is not bad, except perhaps in the couple of places where we have the three definitions of the function.
I think I would be inclined to make it look MORE like two overloads and an implementation. Something like,

Get the specified data provider (returns undefined in the initial render while it is being created, and again in an additional render after it has been disposed)
...first overload...

Return the same Data Provider passed in (this overload makes it easier for some clients to follow the rules of hooks)
...second overload...

Implementation of the two overloads:
...the implementation...

But it's also OK as is...this is just an idea you can use if you think it helps. (The approach has some appeal for useData, too, but there it would be harder to do it without repeating a lot.)

I adjusted the JSDocs to properly give full information for the different overloads of these hooks.

Your comment also made me realize I wasn't memoizing NetworkObjectService.get at all, which needs to happen so we don't create multiple of the same network object. As such, I made that adjustment as well. Thanks!

…es stronger

tjcouch-sil

Reviewable status: 3 of 56 files reviewed, 3 unresolved discussions (waiting on @irahopkinson, @JohnThomson, @lyonsil, and @param)

extensions/hello-someone/hello-someone.js line 46 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Good call. Please leave this comment here so I remember to do this after merging, but I think it may be best to do all the reviewing we want before I merge and have to reformat everything.

Done.

lyonsil

Reviewable status: 3 of 56 files reviewed, 3 unresolved discussions (waiting on @irahopkinson, @JohnThomson, and @param)

irahopkinson

Reviewed 9 of 9 files at r9, 38 of 46 files at r11, 6 of 8 files at r12, 5 of 5 files at r13, 4 of 4 files at r14, all commit messages.
Dismissed @JohnThomson from 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tjcouch-sil)

src/renderer/components/papi-components/text-field.component.tsx line 47 at r14 (raw file):

  className?: string;
  /**
   * Starting value for the text field if it is not controlled

I want to register my objection to adding unrelated (out of scope) items to any PR let alone an already large PR.

This is a non-blocking comment and I'm reviewing them because this PR is overdue for merging, but I felt I need to mention this.

tjcouch-sil

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tjcouch-sil)

src/renderer/components/papi-components/text-field.component.tsx line 47 at r14 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I want to register my objection to adding unrelated (out of scope) items to any PR let alone an already large PR.

This is a non-blocking comment and I'm reviewing them because this PR is overdue for merging, but I felt I need to mention this.

I think it is a good idea to keep unrelated stuff out of the pr. I wasn't really thinking too deeply about how related the component stuff was. I put a text field in for testing, and I realized immediately that it was not controllable. As such, I went and fixed it and used it. But I suppose it makes sense to just keep using a normal input field for the PR and change it in a follow-up. Will keep this in mind for the future. Thanks!

tjcouch-sil added 18 commits March 21, 2023 18:51

Added DataProviderService and example data provider

bfb2510

Changed arrow functions to class methods to make sure either way works

e5ca8af

Split off IDataProviderEngine that extensions implement to provide da…

11c667f

…ta provider functionality, but papi will create a data provider to wrap that engine to provide updates

Started working on selector events

c9df9cf

Revert "Started working on selector events" - trying another method to

50496e3

get updates instead of selector events This reverts commit c9df9cf.

Added subscribe to DataProvider for getting data updates

3b384bb

Stopped data provider update events from irrelevant data changes by d…

79247bd

…eep comparing

Added options to configure data provider subscription, fixed greeting…

af582d2

…s provider indicating it updated when it did not

Fixed usePromise not working with returning functions

84419fd

Added useEventAsync for async (un)subscribers, allow undefined passed…

c409abe

… to useEvent

Added first-pass useData hook

e6fe83a

Added Quick Verse data provider, test useData hook

e01e77c

Updated useData so it returns similar to useState, added default valu…

911839b

…e, minor fixes

Added isLoading on useData so React knows when the data is correct

ab8997d

Debounced verse reference input, subscribed to quick verse updates on…

cbf917b

… webview

Added latest-edited scripture selector to quick verse

d370c16

Added IDataProviderEngine.forceUpdate which allows the DPE to choose …

b56f345

…to send updates in other contexts beside after a successful set

Split Data Provider TData into TGetData and TSetData because I alread…

d6da885

…y found a situation where they are different

tjcouch-sil force-pushed the 59-data-provider branch from b4e4284 to 9312feb Compare April 1, 2023 03:30

Split useDataProvider out into a new hook from useData, tested arbitr…

1fd1c95

…ary method on data provider engine

tjcouch-sil force-pushed the 59-data-provider branch from 9312feb to 1fd1c95 Compare April 1, 2023 03:33

irahopkinson requested changes Apr 3, 2023

View reviewed changes

tjcouch-sil commented Apr 3, 2023

View reviewed changes

JohnThomson reviewed Apr 3, 2023

View reviewed changes

Changed GreetingsDataProvider to an object for testing, other code re…

21c8f6d

…view cleanup

tjcouch-sil commented Apr 3, 2023

View reviewed changes

Fixed couple of deep copy update bugs, misc code review tweaks

ed80cbd

irahopkinson requested changes Apr 5, 2023

View reviewed changes

tjcouch-sil commented Apr 5, 2023

View reviewed changes

JohnThomson reviewed Apr 10, 2023

View reviewed changes

tjcouch-sil added 3 commits April 10, 2023 13:34

Fixed empty object not triggering subscriber updates on first update,…

9956582

… made useDataProvider always return dataProvider once it has been retrieved instead of not returning when disposed, explained get vs subscribe more

Refactored useDataProvider so it now accepts its own return

1000fcc

Improved documentation for useDataProvider and useData

217488f

tjcouch-sil commented Apr 10, 2023

View reviewed changes

irahopkinson reviewed Apr 10, 2023

View reviewed changes

tjcouch-sil commented Apr 11, 2023

View reviewed changes

JohnThomson reviewed Apr 11, 2023

View reviewed changes

Made network objects more thread-safe by using the request registrati…

cb86ffc

…ons as the check for if one already exists with this id

tjcouch-sil commented Apr 11, 2023

View reviewed changes

Stopped returning isDisposed from useDataProvider hook

09748ec

JohnThomson reviewed Apr 11, 2023

View reviewed changes

irahopkinson reviewed Apr 11, 2023

View reviewed changes

tjcouch-sil added 5 commits April 11, 2023 17:03

Added useDataProvider to papi.react.hooks

2420400

Improved JSDocs for useData and useDataProvider

23bd230

Memozied NetworkObjectService.get when sending promises so it does no…

8090ce8

…t try to create multiple

Removed getResourcesPath command to stop that annoying pop-up error

66f53e5

Added check to make sure we aren't making multiple of the same networ…

0a964a8

…k object like we were before

tjcouch-sil commented Apr 11, 2023

View reviewed changes

tjcouch-sil added 5 commits April 12, 2023 09:47

Merge remote-tracking branch 'origin/main' into 59-data-provider

67c1742

Renamed data provider files to match new convention, other lint fixes

d9d5483

Fixed types on papi react stuff, fixed Text Field not being controllable

915bcbc

Added MUI credit

201b50e

Changed logger.log to logger.info, enforced papi react imports as typ…

4bb2809

…es stronger

tjcouch-sil commented Apr 12, 2023

View reviewed changes

lyonsil approved these changes Apr 12, 2023

View reviewed changes

irahopkinson approved these changes Apr 12, 2023

View reviewed changes

tjcouch-sil commented Apr 13, 2023

View reviewed changes

tjcouch-sil merged commit 5a5a964 into main Apr 13, 2023

tjcouch-sil deleted the 59-data-provider branch April 13, 2023 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Data Provider API #122

Create Data Provider API #122

tjcouch-sil commented Apr 1, 2023 •

edited

Loading

irahopkinson left a comment

tjcouch-sil left a comment

JohnThomson left a comment

JohnThomson left a comment

tjcouch-sil left a comment

irahopkinson left a comment

tjcouch-sil left a comment

JohnThomson left a comment

tjcouch-sil left a comment

irahopkinson left a comment

tjcouch-sil left a comment

JohnThomson left a comment

tjcouch-sil left a comment

JohnThomson left a comment

irahopkinson left a comment

tjcouch-sil left a comment

tjcouch-sil left a comment

lyonsil left a comment

irahopkinson left a comment

tjcouch-sil left a comment

Create Data Provider API #122

Create Data Provider API #122

Conversation

tjcouch-sil commented Apr 1, 2023 • edited Loading

irahopkinson left a comment

Choose a reason for hiding this comment

tjcouch-sil left a comment

Choose a reason for hiding this comment

JohnThomson left a comment

Choose a reason for hiding this comment

JohnThomson left a comment

Choose a reason for hiding this comment

tjcouch-sil left a comment

Choose a reason for hiding this comment

irahopkinson left a comment

Choose a reason for hiding this comment

tjcouch-sil left a comment

Choose a reason for hiding this comment

JohnThomson left a comment

Choose a reason for hiding this comment

tjcouch-sil left a comment

Choose a reason for hiding this comment

irahopkinson left a comment

Choose a reason for hiding this comment

tjcouch-sil left a comment

Choose a reason for hiding this comment

JohnThomson left a comment

Choose a reason for hiding this comment

tjcouch-sil left a comment

Choose a reason for hiding this comment

JohnThomson left a comment

Choose a reason for hiding this comment

irahopkinson left a comment

Choose a reason for hiding this comment

tjcouch-sil left a comment

Choose a reason for hiding this comment

tjcouch-sil left a comment

Choose a reason for hiding this comment

lyonsil left a comment

Choose a reason for hiding this comment

irahopkinson left a comment

Choose a reason for hiding this comment

tjcouch-sil left a comment

Choose a reason for hiding this comment

tjcouch-sil commented Apr 1, 2023 •

edited

Loading