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

Filter works incorrectly if fields are splitted by several log actions #115

Open
LexxFedoroff opened this issue Aug 16, 2024 · 4 comments
Open

Comments

@LexxFedoroff
Copy link

Hello, I've debugged this issue logux/examples#21 and found that there is a bug in the filters.
I can reproduce it with a simple test:

it('filter works correctly if sent not all fields', async () => {
  let client = new TestClient('10')
  await client.connect()
  client.log.keepActions()

  let posts = createFilter(client, Post, { authorId: '1'})
  let unbind = posts.listen(() => {})
  await allTasks()

  await client.server.sendAll({ channel: 'posts/1', type: 'logux/subscribed' })
  await client.server.sendAll({
    fields: { title: 'A' },
    id: '1',
    type: 'posts/created'
  })
  await client.server.sendAll({
    fields: { authorId: '1' },
    id: '1',
    type: 'posts/changed'
  })
  await allTasks()

  expect(ensureLoaded(posts.get()).list).toEqual([
    { id: '1', isLoading: false, title: 'A', authorId: '1' },
  ])
  unbind()
})

I want to fix it but I need some information on how that should work.
As I understand we need to accumulate log actions for an object until we receive the fields required for a filter.
What is the best way to achieve this?
Version: 0.21.1

@ai
Copy link
Member

ai commented Aug 16, 2024

Hm. I thought about this case.

This block should do the stuff
https://github.com/logux/client/blob/main/create-filter/index.js#L280-L291

Can you investigate why it doesn’t work?

@LexxFedoroff
Copy link
Author

I've investigated and I think the problem comes from the commit e2dac17
Currently, the sync map is created from the last received action but this action doesn't contain all necessary fields.
@euaaaio what do you think?

@euaaaio
Copy link
Member

euaaaio commented Aug 19, 2024

I won't be able to review it closely today.

I only remember that we had a problem with the implementation, which was somehow very different from how it was designed. Could it be that the bug is in the example repository?

@LexxFedoroff
Copy link
Author

Could it be that the bug is in the example repository?

not sure, I think the bug is in the createFilter method
I'm a newbie in the logux framework and I don't know exactly how it should work but I see two options:

  • a filter must accumulate all changes of the map and when any of the filter's fields comes then creates a sync map with all changes. In this case, there are no extra sync operations but we spend more memory to keep unnecessary data.
  • a filter listens to changes of the map and when any of the filter's fields comes then creates an empty sync map that will sync again. In this case, there are extra sync operations but we don't spend more memory to keep unnecessary data.

the second option was implemented before the commit e2dac17 and I tend that was correct

LexxFedoroff pushed a commit to LexxFedoroff/logux-client that referenced this issue Sep 17, 2024
LexxFedoroff pushed a commit to LexxFedoroff/logux-client that referenced this issue Sep 17, 2024
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

3 participants