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

A proper way of update/re-render single item (using redux) #121

Closed
soulfly opened this issue Jan 17, 2020 · 16 comments
Closed

A proper way of update/re-render single item (using redux) #121

soulfly opened this issue Jan 17, 2020 · 16 comments

Comments

@soulfly
Copy link

soulfly commented Jan 17, 2020

We are building a chat app using ngx-ui-scroll

Seems we have an issue with understanding of how to properly do one thing

Let's say we have data source with 50 messages

Now a user sends 2 new messages, so we call adapter.append to show them, all is good

Then we have messages states, so by default the state is sent and a user can receive a delivered state signal per each message.

So once we receive a delivery signal - we update a special field of message in redux, which causes our subscription to get an updated messages array from redux:

this.conversationRepo.getSelectedConversationMessages().subscribe(messages => {
   // 50 messages
})

So we got 2 signals about delivered state (for our new messages we sent) and then got our subscription called 2 times as well

And here we have 2 questions:

  1. if we want to re-render a single message item to update message.state mark in UI, how we can do it? I see adapter.reload which should be a proper way, but it does not receive a single item index to reload, but a startIndex instead which will cause more items to be reloaded

  2. Is there a way just to connect a redux to ngx-ui-scroll and then it will do all it's job what changed and need to be reloaded and what's not ?

As an alternative idea - my initial idea was to call adapter.reload() all the time we have a redux upgraded (w/o startIndex) and also to have a new method in adapter shouldReload which will then determine should an item be reloaded or not

    this.datasource = new Datasource ({
      get: (index, count, success) => {
        const data = this.getAdpterData(index, count).reverse();
        success(data);
      },
      shouldReload: (index) => {
          return true/false;
       } 
   });

Could you please let me know your thoughts re this

@dhilt
Copy link
Owner

dhilt commented Jan 18, 2020

@soulfly Thanks for the interesting issue! Using redux means the item data (as a part of the app state) would become a new object during an update you want the ui-scroll to take into the account. And here's a problem, because the ui-scroll does not have a mechanism of synchronisation the items he already got and rendered except by an object reference. I mean if your update would have not introduce a new data item object, then no any additional efforts have to be done to spread that update from the app to the ui-scroll. To illustrate this assertion I would ask you to take a look to this demo: angular-ngx-ui-scroll-1-1-0-beta-update-by-reference.

This suggests one of the possible workarounds. You have your items data in the app store, fine. Then we may build an additional layer between the app state data and the ui-scroll datasource, a kind of cache. This cache should present an array which has never to be destructed. You may add new items to the cache, you should not remove items, items updating is allowed only at the properties level (references must not be lost):

this.cache = [] // initialization is the only high-level assignment
this.cache.push({ id: 1, text: 'first item' }) // non-destructive update of the list
this.cache.push({ id: 2, text: 'second item' })
this.cache.find(item => item.id === 1).text = 'first item (changed)' // non-destructive update of the item

Here and below I assume that any item consists of unique and immutable id property and random text property which presents the item's data and could be changed. The idea is to do not lose any references of the items going to the ui-scroll. When you request items for the ui-scroll datasource (inside get method), you may inject cache logic before success call:

get: (index, count, success) => {
  const data = this.getAdapterData(index, count).reverse();

  const _data = []
  data.forEach(item => {
    let result = this.cache.find(_item => _item.id === item.id)
    if (result) {
      result.text = item.text // non-destructive update will keep the item reference
    } else {
      result = item
      this.cache.push(result)
    }
    _data.push(result)
  );

  success(_data) // pass to the ui-scroll cached data which holds the old items references
}

At last we need to provide on-flight updates

/*app state items observable*/.subscribe(items => {
  items.forEach(item => {
    const found = this.cache.find(_item => _item.id === item.id)
    if (found) { // it should be found, I guess, but let this protection remain
      found.text = item.text; // non-destructive update
    }
  })
})

That's it, I hope it's clear enough to try to implement in the real environment. Using javascript Map for the cache implementation might drastically improve the approach.

Also, I'd like to think a bit what can be done on the ui-scroll end to improve the situation. Special updating API development might consume a lot of time, but if it is going to be a popular use case...

@soulfly
Copy link
Author

soulfly commented Jan 20, 2020

@dhilt thank you for the detailed explanation

I think I got the idea.. let me try to implement it and see

@soulfly
Copy link
Author

soulfly commented Jan 20, 2020

The only one thing I do not like is this for loop cycle

/*app state items observable*/.subscribe(items => {
  items.forEach(item => {
    const found = this.cache.find(_item => _item.id === item.id)
    if (found) { // it should be found, I guess, but let this protection remain
      found.text = item.text; // non-destructive update
    }
  })
})

In a case I have 100 messages, even a single message updated in redux will produce a 100 cycles each time

@soulfly
Copy link
Author

soulfly commented Jan 20, 2020

@dhilt anyway I think there should be more integrated API with redux, as Redux is pretty standard approach these days in web apps

@soulfly
Copy link
Author

soulfly commented Jan 20, 2020

@dhilt there is actually an issue with keeping data as non changable

We have a 'MessageComponent' for each message display and we are passing a message as an input

export class MessageComponent implements OnDestroy, AfterViewInit, OnInit, OnChanges {
  @Input() message: Message | any; 

and if now to keeping a data as non changable then it does not fire a 'ngOnChanges' callback as the message bounded to component did not change

There is a workaround for it - to pass all the messages fields that can be changed as separated input fields, that's what I'm going to try now...

@soulfly
Copy link
Author

soulfly commented Jan 22, 2020

@dhilt ok I think your suggestion works well, thank you for the advice 👍

Anyway, I would like to raise another case with Redux, which is a bit tricky to handle and would like to ask for your opinion

So let's say we have same subscription:

messages: Message[] = [];

this.conversationRepo.getSelectedConversationMessages().subscribe(messages => {
    // do some stuff
    // ...    

   this.messages = messages;
})

And now, we have mainly 2 operations which cause this subscription to fire:

  1. send/receive messages, where we need to append 1 message (or sometimes more than 1)
  2. load more, where we need to prepend X messages

So here is what I came up with:

this.conversationRepo.getSelectedConversationMessages().subscribe(messages => {
      // append or prepend?

      if (this.messages.length > 0) {
        if (messages.length !== this.messages.length) {

          // append
          if (messages[0].id !== this.messages[0].id) {
            const newMessages = messages.slice(0, messages.length - this.messages.length);
            const dataToAppend = [];
            newMessages.forEach(msg => {
              const data = this.addMessageToDataSourceCache(msg);
              dataToAppend.push(data);
            });
            this.doAdapterAppendAndScrollBottom(dataToAppend);

          // prepend
          } else {
            const totalLoadedByLoadMore = messages.length - this.messages.length;
            const sliceSize = totalLoadedByLoadMore < 10 ? totalLoadedByLoadMore : 10;

            // prepend (only 10, the rest will be virtualized)
            const newMessages = messages.slice(this.messages.length, this.messages.length + sliceSize);
            const dataToPrepend = [];
            newMessages.forEach(msg => {
              const data = this.addMessageToDataSourceCache(msg);
              dataToPrepend.push(data);
            });
            this.doAdapterPrepend(dataToPrepend);
          }
        } else {
          console.log("[ChatWindowComponent][DataSource] this.messages status updates");
          messages.forEach(msg => {
            const found = this.messagesCached[msg.id];
            if (found) {
              found.messageStatus = msg.status;
              found.messageTimestamp = msg.timestamp;
              found.message = msg;
              found.messageAttachment = msg.attachment;
            }
          });
        }
      }
})

As you can see, to detect is it needs to be appended or prepended is a bit tricky.

For append I just get first element from old and new array and compare its id - if it's different then it needs to be appended. If it's same then this is probably loaded more messages.

Please let me know your feedback, maybe you have a diff solution in mind

@dhilt
Copy link
Owner

dhilt commented Jan 22, 2020

@soulfly Well, the structure of your repo subscription source forces to have such a complicated handler logic, but the logic itself does make sense. Regarding to prepend + virtualizing, you may take a look to PR #119 which provides some functionality that might be helpful (see "Prepeding and virtualizing" section).

Also, what if the Adapter.fix(options) method will be extended to accept an updater option allowing to make changes over the buffered items? At the app component level it might look like

const updatedItem = { id: 15, text: 'new text for item 15' };

this.datasource.adapter.fix({
  updater: item => {
    if (item.id === updatedItem.id) {
      item.text = updatedItem.text;
    }
  }
});

The updater function will be called for each item in the ui-scroll buffer and this way you'll be able to update necessary fields without extra cache-layer on the App side. The implementation of this approach is cheap enough for I can announce new release containing this feature in just 2-3 days. This improvement might be a kind of first step on the way to better on-flight changes handling. What do you think?

@soulfly
Copy link
Author

soulfly commented Jan 23, 2020

@dhilt This probably will make a life easier for a bit,
but
the main issue here is that I do not know what is updatedItem, as what we just receive is an updated messages array

====

I just had some time of thinking about it and came up with an idea to calculate the diffs:

this.conversationRepo.getSelectedConversationMessages().subscribe(messages => {
   const { toAppend, toPrepend, toUpdate, toDelete } = this.getDiffs(this.messages, messages);

  // ...
});

so this will allow to easily call appropriate ngx-ui-scroll methods and then your proposal with updater does make a real sense. Would be great if you can add it and then I will test in a real app :)

@dhilt
Copy link
Owner

dhilt commented Jan 23, 2020

Got it, will try to accomplish it soon.

@dhilt
Copy link
Owner

dhilt commented Jan 27, 2020

@soulfly Adapter.fix({ updater }) is done and released as v1.3.1. Usage notes can be found here. Please try it and feel free to raise new issues!

@dhilt dhilt closed this as completed Jan 27, 2020
@soulfly
Copy link
Author

soulfly commented Jan 27, 2020

Thanks @dhilt,
did some quick tests and seems it works pretty well 👍

I have one small suggestion re performance

Once we found an element by id, it would be great to stop the internal cycle and hence save some cpu, e.g.

doToggleItem(item: MyItem) {
  this.datasource.adapter.fix({
    updater: ({ data }: { data: MyItem }, complete) => {
      if (item.id === data.id) {
        data.isSelected = !data.isSelected;
        complete();
      }
    }
  });
}

what do you think?

@dhilt
Copy link
Owner

dhilt commented Jan 29, 2020

@soulfly Traversing through the inner Buffer seems pretty cheap operation, so I don't see how we'll get richer with this change. Are you going to run fix-updater procedure for many-many times synchronously?

But I would not resist if you open a PR ...

@soulfly
Copy link
Author

soulfly commented Jan 29, 2020

Not so many.. but

It's more about chat apps

E.g. I send a message
and then receive 'sent' then 'delivered' then 'read' status,
so for every single message I sent I need to run this updater to update a tick mark:

  • pending -> sent
  • sent -> delivered
  • delivered -> read

@dhilt
Copy link
Owner

dhilt commented Jan 29, 2020

@soulfly The time needed for processing 1 status update is much less than the time needed for receiving this update due to async nature of receiving. You'll never get the difference with and without the optimization you suggested.

I'm still open for it but let it be a low priority. Also, what do you think on another syntax:

doToggleItem(item: MyItem) {
  this.datasource.adapter.fix({
    updater: ({ data }: { data: MyItem }) => {
      if (item.id === data.id) {
        data.isSelected = !data.isSelected;
        return true;
      }
    }
  });
}

This way we consider the updater method as a predicate whose true value should break the traversing.

@soulfly
Copy link
Author

soulfly commented Jan 30, 2020

I agree it's not a high prio, please just put it into your queue

the prosed syntax with 'return true' also works, I like it 👍

@dhilt
Copy link
Owner

dhilt commented Jan 30, 2020

@soulfly I opened new issue: #133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants