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

query's input stream doesn't trigger the query callback #35

Closed
mtt-artis opened this issue Aug 22, 2022 · 11 comments
Closed

query's input stream doesn't trigger the query callback #35

mtt-artis opened this issue Aug 22, 2022 · 11 comments
Labels
help wanted Extra attention is needed

Comments

@mtt-artis
Copy link

mtt-artis commented Aug 22, 2022

I have a project with angular 14 and I'd like to add rx-query.
I have several components on the same page whose data is retrieved from an API.

Query$ = query('query', this.activatedRoute.queryParams.pipe(map(({id, currency}) => ({id, currency}))), ({id, currency}) => this.repo.getData(id, currency) )

Unfortunately, the query's input stream doesn't trigger the query callback.
I fixed it by changing asapScheduler to asyncScheduler. but I don't know enough rxjs to understand the origin of the problem.

Do you expect a side effect from that?
If not, can I create a PR?

@SchlammSpringer
Copy link

thxs for the hint, tried it and it works, so for me the same question is, what are the side effects?
Or could it be also a problem because of this Bug:

ReactiveX/rxjs#6747

@mtt-artis
Copy link
Author

mtt-artis commented Aug 31, 2022

this might be the problem yes,

I made a stackblitz where you can see the problem starts with rxjs 7.5 (7.4 is fine).

I'm on a project with more than 40 queries and rx-query behaves as i want with the asyncScheduler.

@SchlammSpringer
Copy link

for us the downgrade to rx.js 7.4 is working, with a little jest config editing.
nrwl/nx#7844 (comment)

Still the best solution would be.

  • pull request to asyncScheduler / with new peer dependencies
  • wait until the bug is fixed and then only pull request with new peer depencies

@timdeschryver could you give us a hint which solution/pull request we should focus on.

@timdeschryver
Copy link
Owner

@SchlammSpringer I prefer the first option, but I suspect some tests will fail (If I remember correctly). Feel free to create a PR.
I was also thinking to make the scheduler configurable, which could be useful in both versions.

@timdeschryver timdeschryver added the help wanted Extra attention is needed label Aug 31, 2022
@mtt-artis
Copy link
Author

The asyncScheduler does not guarantee the order of events.
This can be seen particularly on the "can refresh on demand" test where the result is messed up :

// at rx-query/__tests__/query.test.ts:501
it.only('can refresh on demand', async () => {
    const values = [];
    let i = 20;

    for await (const value of eachValueFrom(
        query('test', () => of(i++)).pipe(takeWhile(() => i <= 24, true)),
    )) {
        refreshQuery('test');
        values.push(value);
    }
    console.log(valuesWithoutMutate(values))
    // ...
})
// [
//   { status: 'loading', retries: 0 },
//   { status: 'success', data: 20 },
//   { status: 'refreshing', data: 20, retries: 0 },
//   { status: 'refreshing', data: 20, retries: 0 },
//   { status: 'success', data: 21 },
//   { status: 'success', data: 22 },
//   { status: 'refreshing', data: 21, retries: 0 },
//   { status: 'refreshing', data: 22, retries: 0 },
//   { status: 'refreshing', data: 22, retries: 0 },
//   { status: 'success', data: 23 },
//   { status: 'success', data: 24 }
// ]

This behavior breaks several tests...

@SchlammSpringer
Copy link

Same result here.
But is the execution order really important?
In the tests there is also an isolation problem with the
resetQueryCache.
Single test execution behaves different than suite execution.
First impression from us, change to asyncScheduler is not a viable quickfix, or we don´t understand the cache implementation good enough.

@timdeschryver
Copy link
Owner

Oh shoot.. I accidently pushed some changes to master 😅
I canceled the workflow to prevent a release.
Could you try the version on master please?

@SchlammSpringer
Copy link

SchlammSpringer commented Sep 2, 2022

@timdeschryver thanks for the push. First quick impression, still strange behavior. We will investigate further.

Little Update to what is "strange": Refresh and delete are not working properly and with a huge time offset, in a list of Rick and Morty characters.

Same behavior with refreshQuery in our app.

@timdeschryver
Copy link
Owner

Thanks @SchlammSpringer .
What about #37 ?
It seems to be okay here, but I still want the tests to pass 😅

@SchlammSpringer
Copy link

@timdeschryver thxs for the PR => #37 (comment)

@timdeschryver
Copy link
Owner

Should be resolved in v2.0.1
If that's not the case feel free to open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants