Skip to content
This repository was archived by the owner on May 19, 2025. It is now read-only.

fix firebase query returns 2 sets for #263#270

Merged
tjmonsi merged 5 commits intoFirebaseExtended:masterfrom
tjmonsi:263-fix-firebase-query-returns-2-sets
Sep 26, 2017
Merged

fix firebase query returns 2 sets for #263#270
tjmonsi merged 5 commits intoFirebaseExtended:masterfrom
tjmonsi:263-fix-firebase-query-returns-2-sets

Conversation

@tjmonsi
Copy link
Copy Markdown
Contributor

@tjmonsi tjmonsi commented Sep 21, 2017

Fixing #263

I saw that the __queryChanged is being called thrice, making creation of event listeners called thrice. Currently wrapped them in a checker if both query and oldQuery exists with this idea

  1. __queryChanged is called in the attached phased
  2. __queryChanged is called when any time a new query is created on calculated in the __computeQuery

Can anyone give me some feedback on any bugs I missed?

Thanks

Signed-off-by: Toni-Jan Keith Monserrat <tonijanmonserrat@gmail.com>
Signed-off-by: Toni-Jan Keith Monserrat <tonijanmonserrat@gmail.com>
Signed-off-by: Toni-Jan Keith Monserrat <tonijanmonserrat@gmail.com>
@christophe-g
Copy link
Copy Markdown
Contributor

@tjmonsi - thanks a lot for this - I 'll try to test it over during the we!

@tjmonsi
Copy link
Copy Markdown
Contributor Author

tjmonsi commented Sep 21, 2017

@christophe-g give me a heads-up when you're done. Thanks :)

@tbeattysk
Copy link
Copy Markdown

Using this commit I'm consistently getting duplicates.
I agree with the idea of getting rid of "on-value", any idea why it was used in the first place?

I don't have time to explore your code, but here is the test case I was using: https://github.com/tbeattysk/firebase-duplicate-test

@tjmonsi
Copy link
Copy Markdown
Contributor Author

tjmonsi commented Sep 22, 2017

@tbeattysk did you manage to use my branch for your test case?

Signed-off-by: Toni-Jan Keith Monserrat <tonijanmonserrat@gmail.com>
@tjmonsi
Copy link
Copy Markdown
Contributor Author

tjmonsi commented Sep 22, 2017

@tbeattysk I used your repo as a test case using a fix that I added. It's working now (I think).

As for using on-value: see merged PR: #230

@andrewspy
Copy link
Copy Markdown

@tjmonsi I have tested the latest commit (80ee04a), and it's still not working 100% correctly based on my test case #263 (comment). Now I don't get the duplicated result, but if you click the "Add query" fast enough, you'll get empty query result on the first few query before the result is "eventually" return in subsequent query.

I also have another <firebase-query> in my project, which doesn't return any expected result at all.

Signed-off-by: Toni-Jan Keith Monserrat <tonijanmonserrat@gmail.com>
@tjmonsi
Copy link
Copy Markdown
Contributor Author

tjmonsi commented Sep 23, 2017

I think the original idea of using on instead of once for the value event was a good take. Because the once event is not being called. I don't know why the promise is not being resolved. I think I would try to create a simple repo for it and file a bug in firebase for it.

@tjmonsi
Copy link
Copy Markdown
Contributor Author

tjmonsi commented Sep 23, 2017

@andrewspy can you check it again?

@andrewspy
Copy link
Copy Markdown

@tjmonsi I am happy to report that it is finally working on both my test case and my project.

Thanks!

@tjmonsi
Copy link
Copy Markdown
Contributor Author

tjmonsi commented Sep 25, 2017

@christophe-g any report?

@christophe-g
Copy link
Copy Markdown
Contributor

@tjmonsi - looks good! thanks and sorry for the delay (... quite busy ...)

@tjmonsi tjmonsi merged commit 645371a into FirebaseExtended:master Sep 26, 2017
@FluorescentHallucinogen
Copy link
Copy Markdown
Contributor

@tjmonsi Please release the new version 2.2.1.

@tjmonsi
Copy link
Copy Markdown
Contributor Author

tjmonsi commented Oct 2, 2017

@FluorescentHallucinogen just released 2.2.1... sorry for the delay

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants