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

bug(populate): isLoaded returns true when populated children are still loading #121

Closed
marekolszewski opened this Issue May 4, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@marekolszewski

marekolszewski commented May 4, 2017

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
For populated queries, isLoaded returns true when the base query completes, even if the populated child queries are still outstanding.

What is the expected behavior?
Ideally it would return false while the populated children are being loaded. For backwards compatibility, it may make sense for isLoaded to take an extra parameter such as 'waitForPopulate'.

@prescottprue prescottprue added the bug label May 4, 2017

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue May 4, 2017

Owner

@marekolszewski This is indeed expected behavior, and if you are seeing it, it is for sure a bug.

Which version are you using?

Can you possibly provide a repo where the bug can be reproduced? Feel free to message me directly on gitter instead if the repo is private.

Owner

prescottprue commented May 4, 2017

@marekolszewski This is indeed expected behavior, and if you are seeing it, it is for sure a bug.

Which version are you using?

Can you possibly provide a repo where the bug can be reproduced? Feel free to message me directly on gitter instead if the repo is private.

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue May 5, 2017

Owner

@marekolszewski I was able to replicate. It seems that due to the order of dispatches in the query action, the other actions are dispatched after the main data is update (even though all of the data has been retrieved from Firebase already).

Going to experiment with switching the order of these and see if that helps. The goal being that the main data (what will be used as the path within populatedDataToJS) is updated last, meaning that all of the other data needed for population will already be in redux. That should hopefully keep isLoaded from firing until populated data is available.

Will keep this issue updated with progress.

Owner

prescottprue commented May 5, 2017

@marekolszewski I was able to replicate. It seems that due to the order of dispatches in the query action, the other actions are dispatched after the main data is update (even though all of the data has been retrieved from Firebase already).

Going to experiment with switching the order of these and see if that helps. The goal being that the main data (what will be used as the path within populatedDataToJS) is updated last, meaning that all of the other data needed for population will already be in redux. That should hopefully keep isLoaded from firing until populated data is available.

Will keep this issue updated with progress.

@marekolszewski

This comment has been minimized.

Show comment
Hide comment
@marekolszewski

marekolszewski May 5, 2017

@prescottprue Sounds good!

For anyone else encountering this issue, a simple workaround is to just do a typeof on the field being populated and waiting until it becomes an object.

marekolszewski commented May 5, 2017

@prescottprue Sounds good!

For anyone else encountering this issue, a simple workaround is to just do a typeof on the field being populated and waiting until it becomes an object.

@marekolszewski

This comment has been minimized.

Show comment
Hide comment
@marekolszewski

marekolszewski May 5, 2017

@prescottprue BTW, I also just noticed that adding type: 'once' to my queries is also breaking population for me. I wonder if it's related to this.

marekolszewski commented May 5, 2017

@prescottprue BTW, I also just noticed that adding type: 'once' to my queries is also breaking population for me. I wonder if it's related to this.

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue May 5, 2017

Owner

@marekolszewski I don't believe it is related. once does not currently support populate, but this should be added to the roadmap for sure.

I agree with type checking. It is how my team and I decided to work around it for now.

Owner

prescottprue commented May 5, 2017

@marekolszewski I don't believe it is related. once does not currently support populate, but this should be added to the roadmap for sure.

I agree with type checking. It is how my team and I decided to work around it for now.

@prescottprue prescottprue self-assigned this May 5, 2017

prescottprue added a commit that referenced this issue May 5, 2017

Roadmap updated with coming improvements for v1.4.0
* The following issues were added for release in v1.4.0
  * #121
  * #122
* setCustomParameters support added for release in v1.4.0

prescottprue added a commit that referenced this issue May 5, 2017

notParsed query parameter. isLoaded returns true issue (#121)
* `notParsed` query parameter added
* #121 addressed - dispatch of main data called after child dispatches
have already been called
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue May 5, 2017

Owner

I believe I may have a fix for this, and it should make it into v1.4.0-rc.2. Testing it a bit more, but will hopefully be ready for release tomorrow. This ticket will continued to be updated until it is released into a v1.4.0 (non pre-release version).

As mentioned in the roadmap, there are future plans for making this configurable.

Owner

prescottprue commented May 5, 2017

I believe I may have a fix for this, and it should make it into v1.4.0-rc.2. Testing it a bit more, but will hopefully be ready for release tomorrow. This ticket will continued to be updated until it is released into a v1.4.0 (non pre-release version).

As mentioned in the roadmap, there are future plans for making this configurable.

@prescottprue prescottprue added this to the v1.4.0 milestone May 5, 2017

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue May 6, 2017

Owner

v1.4.0-rc.2 includes the fix for this. I tested plenty, but if anyone else get a chance, post your results.

Owner

prescottprue commented May 6, 2017

v1.4.0-rc.2 includes the fix for this. I tested plenty, but if anyone else get a chance, post your results.

@prescottprue prescottprue changed the title from isLoaded returns true when populated children are still loading to bug(populate): isLoaded returns true when populated children are still loading May 16, 2017

prescottprue added a commit that referenced this issue May 17, 2017

Version v1.4.0 (#133)
#### Features
* `react-native` support (including [complete example](https://github.com/prescottprue/react-redux-firebase/tree/v1.4.0-beta/examples/complete/react-native) app as well as a [create your own recipe](/docs/recipes/react-native.md))
* Server Side Rendering Support ([#72](#72))
* Support for Boilerplates ([#53](#53))
* `pushWithMeta`, `setWithMeta`, and `updateWithMeta` methods added - write to firebase with createdAt/updatedAt and createdBy/updatedBy
* Fix for `unWatchEvent` helper dispatch mapping (#82)
* `populatedDataToJS` triggers `isLoaded` to be true only when all data is populated (instead of once for unpopulated data) [#121](#121)
* Support for `provider.setCustomParameters` on external auth providers (i.e. `provider.setCustomParameters({ prompt: 'select_account' })`)
* `notParsed` query param option added for not parsing when using `equalTo` (for searching numbers stored as strings)
* `profileParamsToPopulate` now works for `$key: true` lists (thanks @fej-snikduj)
* `onRedirectResult` config option added (runs when redirect result occurs)


#### Enhancements/Fixes
* Improvements to Material Example
  * Projects route is now protected (using `UserIsAuthenticated` HOC from `utils/router`)
  * Todos list only displays first 8 (first at the top) - shows using ordering query params
  * Most main routes are now sync (more simple)
* Firebase Library dependency updated to [`v3.9.0`](https://firebase.google.com/support/release-notes/js)
* Fix for `unWatchEvent` helper dispatch mapping ([#82](#82))
* Firebase version is no longer fixed ([#109](#109))
* Only used parts of Firebase Library imported (shrinks bundle size)
* `build:size` npm script added to generate size report for minified bundle ([#107](#107))
* `user` and `credential` are now returned from login method (solves [#106](#106))
* `yarn.lock` file added
* Compose tests improved promise handling (better use of chai-as-promised)
* `profileParamsToPopulate` now accepts `key: true` lists - thanks [@fej-snikduj](https://github.com/fej-snikduj)

prescottprue added a commit that referenced this issue May 17, 2017

Version v1.4.0 (#133)
* `react-native` support (including [complete example](https://github.com/prescottprue/react-redux-firebase/tree/v1.4.0-beta/examples/complete/react-native) app as well as a [create your own recipe](/docs/recipes/react-native.md))
* Server Side Rendering Support ([#72](#72))
* Support for Boilerplates ([#53](#53))
* `pushWithMeta`, `setWithMeta`, and `updateWithMeta` methods added - write to firebase with createdAt/updatedAt and createdBy/updatedBy
* Fix for `unWatchEvent` helper dispatch mapping (#82)
* `populatedDataToJS` triggers `isLoaded` to be true only when all data is populated (instead of once for unpopulated data) [#121](#121)
* Support for `provider.setCustomParameters` on external auth providers (i.e. `provider.setCustomParameters({ prompt: 'select_account' })`)
* `notParsed` query param option added for not parsing when using `equalTo` (for searching numbers stored as strings)
* `profileParamsToPopulate` now works for `$key: true` lists (thanks @fej-snikduj)
* `onRedirectResult` config option added (runs when redirect result occurs)

* Improvements to Material Example
  * Projects route is now protected (using `UserIsAuthenticated` HOC from `utils/router`)
  * Todos list only displays first 8 (first at the top) - shows using ordering query params
  * Most main routes are now sync (more simple)
* Firebase Library dependency updated to [`v3.9.0`](https://firebase.google.com/support/release-notes/js)
* Fix for `unWatchEvent` helper dispatch mapping ([#82](#82))
* Firebase version is no longer fixed ([#109](#109))
* Only used parts of Firebase Library imported (shrinks bundle size)
* `build:size` npm script added to generate size report for minified bundle ([#107](#107))
* `user` and `credential` are now returned from login method (solves [#106](#106))
* `yarn.lock` file added
* Compose tests improved promise handling (better use of chai-as-promised)
* `profileParamsToPopulate` now accepts `key: true` lists - thanks [@fej-snikduj](https://github.com/fej-snikduj)
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue May 17, 2017

Owner

v1.4.0 was released with this functionality included, so this is now out of pre-release.

Owner

prescottprue commented May 17, 2017

v1.4.0 was released with this functionality included, so this is now out of pre-release.

@marekolszewski

This comment has been minimized.

Show comment
Hide comment
@marekolszewski

marekolszewski May 18, 2017

@prescottprue Awesome, thanks for fixing this!

marekolszewski commented May 18, 2017

@prescottprue Awesome, thanks for fixing this!

Tomoya32 added a commit to Tomoya32/react-redux-firebase that referenced this issue Aug 22, 2018

Version v1.4.0 (#133)
* `react-native` support (including [complete example](https://github.com/prescottprue/react-redux-firebase/tree/v1.4.0-beta/examples/complete/react-native) app as well as a [create your own recipe](/docs/recipes/react-native.md))
* Server Side Rendering Support ([#72](prescottprue/react-redux-firebase#72))
* Support for Boilerplates ([#53](prescottprue/react-redux-firebase#53))
* `pushWithMeta`, `setWithMeta`, and `updateWithMeta` methods added - write to firebase with createdAt/updatedAt and createdBy/updatedBy
* Fix for `unWatchEvent` helper dispatch mapping (#82)
* `populatedDataToJS` triggers `isLoaded` to be true only when all data is populated (instead of once for unpopulated data) [#121](prescottprue/react-redux-firebase#121)
* Support for `provider.setCustomParameters` on external auth providers (i.e. `provider.setCustomParameters({ prompt: 'select_account' })`)
* `notParsed` query param option added for not parsing when using `equalTo` (for searching numbers stored as strings)
* `profileParamsToPopulate` now works for `$key: true` lists (thanks @fej-snikduj)
* `onRedirectResult` config option added (runs when redirect result occurs)

* Improvements to Material Example
  * Projects route is now protected (using `UserIsAuthenticated` HOC from `utils/router`)
  * Todos list only displays first 8 (first at the top) - shows using ordering query params
  * Most main routes are now sync (more simple)
* Firebase Library dependency updated to [`v3.9.0`](https://firebase.google.com/support/release-notes/js)
* Fix for `unWatchEvent` helper dispatch mapping ([#82](prescottprue/react-redux-firebase#82))
* Firebase version is no longer fixed ([#109](prescottprue/react-redux-firebase#109))
* Only used parts of Firebase Library imported (shrinks bundle size)
* `build:size` npm script added to generate size report for minified bundle ([#107](prescottprue/react-redux-firebase#107))
* `user` and `credential` are now returned from login method (solves [#106](prescottprue/react-redux-firebase#106))
* `yarn.lock` file added
* Compose tests improved promise handling (better use of chai-as-promised)
* `profileParamsToPopulate` now accepts `key: true` lists - thanks [@fej-snikduj](https://github.com/fej-snikduj)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment