Next

[j4hangir]

Fix for tinode/chat#602

[or-else]

Please resolve the conflicts.

[or-else]

Something is wrong with this PR. The diff shows my recent commits. Could you please send a clean PR with just your commits. When you go to https://github.com/tinode/tinode-js/pull/43/files it should show just the changes that you made.

[or-else]

Why do you want to exclude built files?

[j4hangir]

That's a mistake, please exclude that line

[j4hangir]

Something is wrong with this PR. The diff shows my recent commits. Could you please send a clean PR with just your commits. When you go to https://github.com/tinode/tinode-js/pull/43/files it should show just the changes that you made.

Yes I'm aware, I reverted my commit since there were too many a change.

Testing the code right now.

[j4hangir]

@or-else the problem I have right now, after getting the latest version, the _contacts is actually undefined even after calling this.tinode.getMeTopic().contacts(..)}, seems like now you're using the key topic inside cacheMap and _contacts is dangling. Gonna see what's changed and make a new PR.

[j4hangir]

Specifically here: https://github.com/tinode/tinode-js/blob/next/src/tinode.js#L5513

Why not enumerate this._contacts like before? This part seems undone to me.

[or-else]

I've been working on this feature for the last week tinode/chat#443 (item 10).

In order for this feature to work I need to get rid of duplicate caching of topics (and I want to add indexDB cache so they are available on startup). Originally they were cached twice: in Tinode and in TopicMe. I unified the cache so now they are stored once just in Tinode. So, _contacts in TopicMe are gone because they no longer have a purpose, but the method contacts() is still present.

[j4hangir]

I've been working on this feature for the last week tinode/chat#443 (item 10).

In order for this feature to work I need to get rid of duplicate caching of topics (and I want to add indexDB cache so they are available on startup). Originally they were cached twice: in Tinode and in TopicMe. I unified the cache so now they are stored once just in Tinode. So, _contacts in TopicMe are gone because they no longer have a purpose, but the method contacts() is still present.

I got your intention after reading further 👍🏻

For now I added a getter named contactList to TopicMe, can duplicate it for TopicFnd later on, think there _contacts is still being used.

[or-else]

1. Please add a comment in jsdoc style.
2. Please add a semicolon ; in the end of every line to make your code style consistent with the rest of the project.

Thanks!

[or-else]

The code style for if is

if (condition) {
   ....;
}

Please follow the project code style.

[or-else]

Likewise, please follow the project code style.

[or-else]

BTW, this is pretty much the same as contacts(). Essentially you are adding the same method as contacts just under a different name.

[or-else]

So, why add contactList() when there is contacts() which does exactly the same thing?

[j4hangir]

So, why add contactList() when there is contacts() which does exactly the same thing?

It's not the best of solutions, preferably one would like to have a persistent access to a readonly list which gets updated, but, it's also okay to have a stateless list of contacts especially now that they're cached in a not so easily retrievable way.

The rationale is to have a synchronous access, contacts() isn't.

.contactsList or .listOfContacts()

Both can do, I find getter more convenient, as it's not writable neither.

[j4hangir]

To illustrate further, if you have an awaitable function that you'd call within the CB, then the CB needs to be async, which makes the whole contacts() asynchronous.

Another simple, perhaps better fix here could be to see if the CB is awaitable and await it right there, keep the flow synchronous.

[j4hangir]

function isPromise(value) {
  return value && value.then && typeof value.then === 'function';
}

inside contacts() we can do this:

contacts: {
    value: async function(callback, filter, context) {
      this._tinode.cacheMap('topic', (c, idx) => {
        if (c.isComm() && (!filter || filter(c))) {
          let ret = callback.call(context, c, idx);
          if (isPromise(ret)) // <==
              ret = await ret;
        }
      });
    },
    enumerable: true,
    configurable: true,
    writable: true
  }

This naturally needs the contacts() to be async as well, hence, all things taken into account, .contactsList should suffice.

[or-else]

I don't understand.

Here is what you are proposing:

  contactsList: {
    get: function() {
      const ret = []
      for (let idx in this._tinode._cache) {
        if (idx.indexOf('topic:') !== 0) continue
        const c = this._tinode._cache[idx]
        if (!c.isComm()) continue
        ret.push(c)
      }
      return ret
    },
  },

Here is the original method:

  contacts: {
    value: function(callback, filter, context) {
      this._tinode.cacheMap('topic', (c, idx) => {
        if (c.isComm() && (!filter || filter(c))) {
          callback.call(context, c, idx);
        }
      });
    },
  },

The difference is that your method returns an array, while the original calls the callback right away. You can get exactly the same result with the original method:

const ret = [];
me.contacts((c) => ret.push(c));

So, why create confusion by adding a duplicate method?

[j4hangir]

What if inside the CB you needed to await an async function?

In order to make that work, you'd need to make the CB itself async, and JS will happily accept and execute it, but it won't wait for it to finish if you don't use await, hence contacts becomes asynchronous and the code you posted won't work as expected. This is the problem I faced since my database calls are async and I am retrieving last message associated with each contact before populating my UI.

P.s.
That aside, why's there even a CB in the first place I'd ask, if there's a list then there's a list, why not just let the user retrieve it and write their own loop?

[or-else]

What if inside the CB you needed to await an async function?

You can always add an await inside your callback. You can also create an array and iterate over it.

That aside, why's there even a CB in the first place I'd ask, if there's a list then there's a list, why not just let the user retrieve it and write their own loop?

In case of your proposal the return value of contactsList may confuse people that they are getting the master copy of the contact list when in reality they get a snapshot. There is no such confusion in case of a callback.

[j4hangir]

What if inside the CB you needed to await an async function?

You can always add an await inside your callback. You can also create an array and iterate over it.

That aside, why's there even a CB in the first place I'd ask, if there's a list then there's a list, why not just let the user retrieve it and write their own loop?

In case of your proposal the return value of contactsList may confuse people that they are getting the master copy of the contact list when in reality they get a snapshot. There is no such confusion in case of a callback.

You cannot await inside CB, that's a syntax error.

You can await inside the CB only when the CB itself is also async, but then by doing so contacts() becomes asynchronous.

I think you're aware that in JS, an aynac can be called with or without await, and since contacts() isn't aware whether or not the CB is async, it will merely call it and not block it, hence become asynchronous.

If you read above, I also proposed to make contacts() await-aware.

That's more than enough solutions, I stated the reason, the rationale and practical cases.

Having contacts taking a CB and merely looping it doesn't add any value but only complications.

Now, you could:

• Rename, make it obvious a snapshot is returned
• Refactor, use the code above to make contacts return the readonly getter.
  -...

[or-else]

You cannot await inside CB, that's a syntax error.

Works fine:
https://jsfiddle.net/uzm6j53e/

[j4hangir]

You cannot await inside CB, that's a syntax error.

Works fine:

https://jsfiddle.net/uzm6j53e/

The slowMethod is async!!!!

It was said multiple times, if we make the CB async, then the contacts() becomes asynchronous, therefore can't make a let tmp = [] and populate it with the CB as you wrote above.

[j4hangir]

I didn't expect this little thing drag this long, either way there are many ways to "work around" this imposed limitation, but that is what it is at its core, unnecessary.

If your original idea had been to make contacts readonly, I assume a backward-incompatible refactoring with CB-receiving methods into getters would do.

[or-else]

async function slowMethod(name) { // <---- this function is async
  const result = await Promise.resolve(name);
  console.log("Hello ", result);
};

["Alice", "Bob", "Carol"].forEach((name) => {  // <----- this CALLBACK is NOT async.
  slowMethod(name);  //  <<<---- this async function is inside a CALLBACK.
});

The slowMethod is async!!!!

Correct. And it's used inside a callback which is NOT async. And Array.forEach is NOT async. As you can see from the example there is no problem whatsoever to have an async method inside a callback. Just wrap it into another method.

I didn't expect this little thing drag this long

Your proposal does not make sense to me. I cannot see why it is better than the way it's done now. It's different but I see no benefits. It would require refactoring without a clear purpose.

[j4hangir]

async function slowMethod(name) { // <---- this function is async

  const result = await Promise.resolve(name);

  console.log("Hello ", result);

};



["Alice", "Bob", "Carol"].forEach((name) => {  // <----- this CALLBACK is NOT async.

  slowMethod(name);  //  <<<---- this async function is inside a CALLBACK.

});

The slowMethod is async!!!!

Correct. And it's used inside a callback which is NOT async. And Array.forEach is NOT async. As you can see from the example there is no problem whatsoever to have an async method inside a callback. Just wrap it into another method.

I didn't expect this little thing drag this long

Your proposal does not make sense to me. I cannot see why it is better than the way it's done now. It's different but I see no benefits. It would require refactoring without a clear purpose.

In JS one can call an async function but if wanted to await it, the caller function needs to be async as well.

Do you follow this? You can call without awaiting, which means contacts becomes asynchronous, which means, you can't generate the list in the way that you wrote above.

That code will not work IF the CB was async, of course you can do it in multiple steps and complicate things further (work around it)... All for no reason anyways 😀

[or-else]

In JS one can call an async function but if wanted to await it, the caller function needs to be async as well.

I just demonstrated to you how you can do it without making the callback async. Please stop repeating the same thing. You have this very simple example which shows how you can do it. If you think the code snippet does not do what you want, show.

There is no point in going in circles.