Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timing of connect during instantiation #195

Closed
gotjoshua opened this issue Aug 3, 2022 · 13 comments
Closed

Timing of connect during instantiation #195

gotjoshua opened this issue Aug 3, 2022 · 13 comments
Labels
bug properties Applies to the property definition template engine Applies to built-in template engine

Comments

@gotjoshua
Copy link

I am running into some sort of race condition during instantiation, that i don't fully understand how hybrids works in the depths. Hoping someone can shed some light.

So i am wiring up a component to dexie liveQuery observable. It is a simple query that users the rowID attribute to set query subscription that returns all elements that are in that row and sets the result already as the value of one of the other component attributes. It is working quite well except that sometimes the observable uses the default value for rowID instead of the value from the component attributes.

I don't understand how/why the connect function of one attribute gets the default value instead of the value that is given to the component at "instantiation" via an "html" attribute.

I don't have a simple reproduction available yet, but can try to create one of it's really needed to understand the question.

Thanks...

@smalluban
Copy link
Contributor

smalluban commented Aug 4, 2022

There is a difference between the following two examples about what you would have in connect() method (those are parts of render of some parent component).

// Predefined attribute
html`<my-element rowId="1"></my-element>`

// dynamic expression
html`<my-element rowId="${1}"></my-element>`

The first gets predefined attribute, so in connect method when you get host[key], the property is taken from an attribute, which is already set.

In the second example, the element is connect first, then the template engine execute expressions and set values, so the rowId in connect method is not yet initialized.

I think this may happen in your case. I would suggest to avoid using connect method for the Dixie initialization. Rather use get method, so it will be executed only and when the value is needed (and for sure after the template sets the rowId). You can then also react to the rowId changes dynamically, and clear previous subscriptions.

@smalluban smalluban added properties Applies to the property definition template engine Applies to built-in template engine labels Aug 4, 2022
@gotjoshua
Copy link
Author

I think this may happen in your case.
I think so too! totally explains my opservations.

Rather use get method

i did try that, and would love if it can work, but all dexie functions are async.
so i don't think i can do an async query in get, can i?

perhaps a combination of observe and set could work. leaving the get fx undefined ??

anyway i have it working now via an artificial delay in the connect definition.

many thanks for your reply!

@smalluban
Copy link
Contributor

smalluban commented Aug 29, 2022

You can utilize two properties, where one holds the dixie connection, and the second keeps data. It is rather hard to make it with only one, as you need to have reference to the subscription to clean in up.

import { liveQuery } from "dexie";

function liveQueryFactory(db, property = "row", result = "data") {
  return {
    [property]: {
      get: (host, lastValue) => {
        return liveQuery(db.rows.get(lastValue)).subscribe({
          next: (data) => { host[result] = data; },
          error: ???
        });
      },
      set: (host, value, lastValue) => {
        // lastValue is a subscription if defined
        if (lastValue) lastValue.unsubscribe();
      
        // pass the row ID to the getter
        return value;
      },
      connect(host, key) {
        return () => host[key]?.unsubscribe();
      },
      observe() {}, // Ensures that the row property is called if the `row` is only set by the static attribute
    },
    [result]: undefined,
  };
}
import db from "./db";

define({
  tag: "my-element",
  ...liveQueryFactory(db),
  render: ({ data }) => html`
    ${data && data.map(...)}
  `,
});

It might not be obvious what happens above, but the solution works without hacky delay, and it supports dynamic change of the row over time, and it clears the subscription when needed.

However, It looks like Observable is a pattern that is not easy to integrate with the hybrids architecture. I will think about it how to make it simpler. It could be something similar to html.resolve() for promises, so the observable would be resolved inside of the template.

@gotjoshua
Copy link
Author

Many thanks for engaging!...

I don't quite manage to get this working with generic queries.

get: (host, lastValue) => {
        return liveQuery(db.rows.get(lastValue)).subscribe({

I do have a working version of useLiveQueryConnction

i assume that yours is more elegant, but i did not manage to adapt it.

@smalluban
Copy link
Contributor

You would need to move db.rows.get... into the factory argument, like: liveQueryFactory(db, query). Dexie docs show that query is defined separately from the liveQuery. I did not want to make the example less readable.

@gotjoshua
Copy link
Author

need to move db.rows.get... into the factory argument, like: liveQueryFactory(db, query)

i did try that, but without success (although i admit i did not spend much time, as it is working as is ,and i don't know the pitfalls of my current approach)

   [property]: {
      get: (host, lastValue) => {
        return liveQuery(db.rows.get(lastValue)).subscribe({
          next: (data) => { host[result] = data; },
          error: ???
        });
      },
      set: (host, value, lastValue) => {
        // lastValue is a subscription if defined
        if (lastValue) lastValue.unsubscribe();
      
        // pass the row ID to the getter
        return value;
      },

but for understanding's sake, can you explain more the dual usage of lastValue?
It doesn't make sense to me (yet) why i would pass a maybeSubscription to a db.Table.get(lastValue)
or is lastValue never a subscription in get, but only maybe in set?
If that is the case, can you take a moment to articulate why?

@gotjoshua
Copy link
Author

you need to have reference to the subscription to clean in up.

i got this working now (will update with links to the repo after i refine it and push)

but i have a question about cleanup: why do we need to unsubscribe in two places?

/**********
 * sets up reactive rendering using dexie liveQuery subscription
 * usage: ...liveQueryFactory<SchemeEditor>('schemeID', 'colors', (val: number) => () => colTable.where('row').equals(val).toArray())
 *
 * @param {string} dynamicKeyName  the key that will be used dynamically by the liveQuery inner function (queryFxFactory)
 * @param {string} resultKeyName the key that will recieve the result - typically an array to be rendered reactively
 * @param {() => () => Promise<any>} queryFxFactory Function that returns a function that does an async query to be subcribed to
 * @returns two keys to be expandede into the host component
 *********/
export function liveQueryFactory<HostType> (dynamicKeyName = 'dynamicKeyUsedInQuery', resultKeyName = 'dataProp', queryFxFactory = (val) => async () => val) {
  return {
    [dynamicKeyName]: {
      get: (host: HostType, currentVal) => {
        VERBOSE('lq get', { currentVal, queryFx: queryFxFactory })
        return liveQuery(queryFxFactory(currentVal)).subscribe({
          next: (data) => { host[resultKeyName] = data },
          error: error => console.error(error),
        })
      },
      set: (host: HostType, value, lastValue) => {
        if (lastValue) lastValue.unsubscribe() // lastValue is a subscription if defined
        return value // pass the row ID to the getter
      },
      connect: (host: HostType, key) => () => host[key]?.unsubscribe(), // not sure why we need unsubscribe both here and in set
      observe () {}, // Ensures that the row property is called if the `row` is only set by the static attribute
    },
    [resultKeyName]: undefined,
  }
}

One important note:

liveQuery expects a function so the liveQueryFactory requires a functionFactory
that returns a function that uses the current value to do a query...
not a call to a function that returns a promise (like the first proposal from @smalluban)

@smalluban
Copy link
Contributor

You're right, you should pass a function to liveQuery :)

but i have a question about cleanup: why do we need to unsubscribe in two places?

Clean up in set is required, as you set new liveQuery, and old should unsubscribe. Clean up in disconnect (returned function in connect) is for a case when you remove (or move) an element from the DOM. You should remember to clean up everything that might "hang" if the component is removed from the DOM.

@gotjoshua
Copy link
Author

i just ran into a snag with this factory setup...
if i try to access the dynamicKeyName from the host i get an object with the unsubscribe fx. is there a way to get the current value of the property in the content function?

 content: (host: SchemeEditor) => {
    const { schemeID } = host

i expected the above to give me the ID, not

{
schemeID:
  closed: false
  unsubscribe: () => {…}
    length: 0
    name: "unsubscribe"
    ...
}

@smalluban
Copy link
Contributor

smalluban commented Sep 6, 2022

the [dynamicKeyName] when set takes the Id, but then in getter, it returns a liveQuery result. The value pass to set is kind of lost. I thought that you don't need it later, so we can omit creating another property to hold it. If so, you need properties for id, liveQuery, and result data.

Honesty, it is going to be too complex, and I think it should be solved by the library in another way. One of the problems with it, is that it is not yet standardized. However, you may try out using the store to hold your result. It might be much simpler then:

import { store } from 'hybrids';

export function liveQuery(structure, fn) {
  const Model = { id: true, ...structure };
  const liveQueries = new WeakMap();

  function unsubscribe(host) {
    if (liveQueries.has(host)) {
      liveQueries.get(host).unsubscribe();
    }
  }

  return {
    get: (host, lastValue) => {
     if (!lastValue) return undefined;

      if (typeof lastValue !== "object") {
        const id = lastValue;

        liveQueries.set(
          host,
          dixie.liveQuery(() => fn(id)).subscribe({
            next: (result) => {
              store.sync(store.get(Model, id), result);
            },
            error: (error) => console.error(error),
          }),
        );

        return store.get(Model, id);
      }

      return store.get(Model, lastValue.id);
    },
    set: (host, id) => {
      unsubscribe(host);
      return id;
    },
    connect: (host) => () => {
      unsubscribe(host);
    },
  };
}

Then in your component:

define({
  tag: "my-element",
  row: liveQuery({ firstName: "", lastName: "" }, (id) => db.rows.get(id)),
  render: ({ row }) => html`
    <div>${store.ready(row) && html`${row.firstName}`}</div>
  `,
});

If you want to access the passed id, the store model instance always keeps it in row.id.

EDIT: I made some fixes in the snippet, so please use the latest version ;)

@gotjoshua
Copy link
Author

Thanks for taking so much time to dive into this issue @smalluban ! I'll give this new technique a try later this week...

@gotjoshua
Copy link
Author

too complex, and I think it should be solved by the library in another way

I find this an interesting statement... Do you have ideas?

It feels like all of the solutions that we have tried so far are some how workarounds.

Independent of the live query situation, i wonder if the connect call could actually be delayed until the template engine sets all values on the component.

then the template engine execute expressions and set values, so the rowId in connect method is not yet initialized

Although technically understandable, this is rather counter intuitive.

Perhaps a flow chart diagram about what happens when (like the old react life cycle, life saving, diagram) could be helpful.

@smalluban
Copy link
Contributor

smalluban commented Nov 2, 2022

I'm back with some news about the core issue of the discussion here.

I was thinking about the inconsistency between dynamic and static values, and after all, I thought that it is a kind of bug. Compared to more abstract frameworks, components always get the last and current props before you can call useEffect (or something similar).

So... I made a major refactor of the core modules. As the result, from 8.1.6 behavior is consistent, and connect always is called after props can be set by dynamic expression or by static value. Additionally, I made some performance tweaks, so with a heavy page (5k+ components), core code runs like 4x faster ;)

Still, I want to find a cleaner solution for support Observables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug properties Applies to the property definition template engine Applies to built-in template engine
Development

No branches or pull requests

2 participants