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

feat(HOCs): option to re-render HOCs based on auth state change #367

Open
klis87 opened this Issue Dec 27, 2017 · 7 comments

Comments

Projects
2 participants
@klis87

klis87 commented Dec 27, 2017

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

What is the current behavior?
After user login, items should be fetched from database. Currently, browser refresh is required, which shouldn't be necessary.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via codesandbox or similar.

  1. Go to https://www.webpackbin.com/bins/-L1PEJ-wkb21zcAApIG8
  2. Click Login, user is logged in but no todos are loaded
  3. Refresh the page - now todos are loaded, but it required page reload

What is the expected behavior?
Todos should be fetched after user is logged in, without page refresh being required

Which versions of dependencies, and which browser and OS are affected by this issue? Did this work in previous versions or setups?
firebase: 4.8.1, react-redux-firebase: 2.0.0-rc.1

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Dec 27, 2017

Owner

@klis87 This is currently expected behavior, currently HOCs (such as firebaseConnect) only re-render when the props change or the listener paths change. The state based query example shows loading auth in a parent component and passing it as a prop.

Reloading based on auth state change by default can cause unexpected re-renders in other situations (i.e. components wrapped in firebaseConnect not using auth state). That can be pretty troublesome in large applications using firebaseConnect at many levels.

The plan has been to offer a config option for this in the future (something like reloadOnAuthStateChange), but there hasn't been work on it yet.

Owner

prescottprue commented Dec 27, 2017

@klis87 This is currently expected behavior, currently HOCs (such as firebaseConnect) only re-render when the props change or the listener paths change. The state based query example shows loading auth in a parent component and passing it as a prop.

Reloading based on auth state change by default can cause unexpected re-renders in other situations (i.e. components wrapped in firebaseConnect not using auth state). That can be pretty troublesome in large applications using firebaseConnect at many levels.

The plan has been to offer a config option for this in the future (something like reloadOnAuthStateChange), but there hasn't been work on it yet.

prescottprue added a commit that referenced this issue Dec 28, 2017

Docs + Roadmap updates for v2.0.0 release
* Note added to reducer docs about how it is multiple slice reducers
combined with combineReducers
* Link added for separate reducers
* Roadmap updated to include option for re-rendering HOCs based on auth
state change - #367

@prescottprue prescottprue added this to To Do in v2.1.0 via automation Dec 28, 2017

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Dec 28, 2017

Owner

@klis87 For clarity, I included a note about this in the updates of the roadmap for the v2.0.0 release.

Going to switch this issue to a place to track the new feature and move it to the Project for v2.1.0

Owner

prescottprue commented Dec 28, 2017

@klis87 For clarity, I included a note about this in the updates of the roadmap for the v2.0.0 release.

Going to switch this issue to a place to track the new feature and move it to the Project for v2.1.0

@prescottprue prescottprue changed the title from Items from db not updated after user login to feat(HOCs): option to re-render HOCs based on auth state change Dec 28, 2017

@prescottprue prescottprue referenced this issue Dec 28, 2017

Merged

v2.0.0-rc.2 #364

3 of 3 tasks complete

prescottprue added a commit that referenced this issue Dec 28, 2017

v2.0.0-rc.2 (#364)
* fix(firestore): profile update with firestore - #360 - @barnomics
* fix(auth): `loginWithCustomToken` supports passing `profile` parameter - #359
* feat(auth): error messages for not logged using same wording for all methods (i.e. “User must be logged in”)
* feat(docs): `loginWithCustomToken` section updated with note about how `profile` is required in certain cases - #359
* feat(docs): roadmap updated to include option for re-rendering HOCs based on auth state change - #367
* feat(docs): link added to sidebar for each of the separate reducers
* feat(docs): note added to FAQ about why `yarn.lock` file is not included
* fix(test): extended timeout added to unit tests for `login` to prevent fail on slow connections
* feat(docs): note added to reducer docs about how it is multiple slice reducers combined with `combineReducers`
* feat(test): unit tests added for auth/query actions not previously covered (`updateEmail`, `updateAuth`, `orderedFromSnapshot`, etc)
* fix(test): tests simplified by moving reused functions to test utils
* fix(test): remove lint warning caused by `mocha.opts` being ignored
* update(deps): `prop-types` dependency updated from `v15.5.*` to v15.6.*`
* update(deps): tons of dev dependencies updated including `babel-cli`, `babel-core`, `eslint`,  `sinon`, `mocha`, `jsdom`

##### Note

`v2.0.0` syntax is now considered stable 🍾 (multiple production applications have switched from `v1.5.*` to `v2.*.*` following [the migration guide](docs.react-redux-firebase.com/history/v2.0.0/docs/v2-migration-guide.html), and test coverage is better than for `v1.5.*`). Merge conflicts with `master` have also been resolved. All of this means this release will be the final RC released on `next` before moving [`v2.0.0`](https://github.com/prescottprue/react-redux-firebase/tree/v2.0.0) to the `latest` tag on npm (the `latest` tag release will include merging to `master`).
@klis87

This comment has been minimized.

Show comment
Hide comment
@klis87

klis87 Dec 28, 2017

@prescottprue That's awesome this is going to be included in core! For now, I tried to make some workaround:

export default compose(
  firebaseConnect(({ profile }) => [
    { path: '/todos', queryParams: [`equalTo=${profile}`] },
  ]),
  connect(({ firebase: { data: { todos } } }) => ({
    todos,
  })),
)(Todos);

... <Todos profile={profile} />

Unfortunately then no todo is displayed as equalTo doesn't hit anything. Without equalTo, it doesn't work either - todos aren't fetched after logging like in above example - passing profile doesn't change anything. Could you please advise there is anything I am doing wrong?

klis87 commented Dec 28, 2017

@prescottprue That's awesome this is going to be included in core! For now, I tried to make some workaround:

export default compose(
  firebaseConnect(({ profile }) => [
    { path: '/todos', queryParams: [`equalTo=${profile}`] },
  ]),
  connect(({ firebase: { data: { todos } } }) => ({
    todos,
  })),
)(Todos);

... <Todos profile={profile} />

Unfortunately then no todo is displayed as equalTo doesn't hit anything. Without equalTo, it doesn't work either - todos aren't fetched after logging like in above example - passing profile doesn't change anything. Could you please advise there is anything I am doing wrong?

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Dec 28, 2017

Owner

@klis87 If you are using equalTo you have to first provide the parameter which you are trying to check equality with equalTo for (i.e. orderByChild). Otherwise the query will not know what you are trying to look for the equalTo To match.

You need something like:

export default compose(
  firebaseConnect(({ profile }) => [
    {
       path: '/todos',
       queryParams: [
          'orderByChild=createdBy', // parameter which will be check for equality
          `equalTo=${profile}` // the equalTo check
       ]
    },
  ]),
  connect(({ firebase: { data: { todos } } }) => ({
    todos,
  })),
)(Todos);

Other thoughts:

Did you try confirming that data is loaded in a parent component first as shown in the state based query example?

If your profile prop is not yet available, that could cause you to be creating a query that you are not intending to (with a query param 'equalTo=undefined').

Owner

prescottprue commented Dec 28, 2017

@klis87 If you are using equalTo you have to first provide the parameter which you are trying to check equality with equalTo for (i.e. orderByChild). Otherwise the query will not know what you are trying to look for the equalTo To match.

You need something like:

export default compose(
  firebaseConnect(({ profile }) => [
    {
       path: '/todos',
       queryParams: [
          'orderByChild=createdBy', // parameter which will be check for equality
          `equalTo=${profile}` // the equalTo check
       ]
    },
  ]),
  connect(({ firebase: { data: { todos } } }) => ({
    todos,
  })),
)(Todos);

Other thoughts:

Did you try confirming that data is loaded in a parent component first as shown in the state based query example?

If your profile prop is not yet available, that could cause you to be creating a query that you are not intending to (with a query param 'equalTo=undefined').

@prescottprue prescottprue added this to the v2.1.0 milestone Dec 30, 2017

@klis87

This comment has been minimized.

Show comment
Hide comment
@klis87

klis87 Dec 30, 2017

@prescottprue in my case profile is never undefined. For logged out user it is equal to { isLoaded: true, isEmpty: true }, for authenticated { isLoaded: true, isEmpty: true ...rest }. However, I really don't want to use any queryParams, my use case is the following - I have dababase objects which are public (common for all users), but I want them to be loaded only after user signed in, I don't want anything to be fetched for anonymous user.

klis87 commented Dec 30, 2017

@prescottprue in my case profile is never undefined. For logged out user it is equal to { isLoaded: true, isEmpty: true }, for authenticated { isLoaded: true, isEmpty: true ...rest }. However, I really don't want to use any queryParams, my use case is the following - I have dababase objects which are public (common for all users), but I want them to be loaded only after user signed in, I don't want anything to be fetched for anonymous user.

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Dec 31, 2017

Owner

@klis87 Passing as a prop as done in the state based query example should work great for that then.

Owner

prescottprue commented Dec 31, 2017

@klis87 Passing as a prop as done in the state based query example should work great for that then.

@klis87

This comment has been minimized.

Show comment
Hide comment
@klis87

klis87 commented Dec 31, 2017

@prescottprue I try but without any effect. Please see https://www.webpackbin.com/bins/-L1hFJz7jujeHpXvI_oE

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