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(populate): populate ordered data #239

Closed
benoj opened this Issue Aug 15, 2017 · 22 comments

Comments

Projects
None yet
5 participants
@benoj

benoj commented Aug 15, 2017

Is it possible to have ordered data that has been populated?

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 16, 2017

Owner

@benoj No, populatedDataToJS does not currently handle ordered data in v1 (but is a planned feature of populate in v2). You can however do the population from redux by hand since the data is available.

That means that data needed for populating can be gathered with populates in your query config, but you will need to do the mapping yourself.

Something like this if using v1.*.*:

const populates = [{ child: 'owner', root: 'users' }]

@firebaseConnect([
  { path: 'todos', populates}
])
@connect(
  (state => ({
    todos: dataToJS(state.firebase, 'todos'),
    populatedTodos: state.firebase.getIn(['data', 'todos'])
      ? state.firebase
          .getIn(['data', 'todos'])
          .map(todo => ({
            ...todo.toJS(),
            // get owner from users in redux defaulting to id if user not found
            owner: dataToJS(firebase, `users/${todo.get('owner')}`, todo.owner) 
          }))
          .toJS()
      : [],
  })
)

Or like this with v2.*.*:

const populates = [{ child: 'owner', root: 'users' }]

@firebaseConnect([
  { path: 'todos', populates }
])
@connect(
  (({ firebase: { ordered: { todos }, data: { users } }) => ({
    todos,
    populatedTodos: todos
      ? todos.map(todo => ({ ...todo, owner: users[todo.owner] || todo.owner }))
      : [],
  })
)

Obviously this is not as clean as directly using populatedDataToJS in v1.*.* or populate in v2.*.*, but as mentioned, it will be coming.

Always open to pull requests if you end up looking into the source.

Owner

prescottprue commented Aug 16, 2017

@benoj No, populatedDataToJS does not currently handle ordered data in v1 (but is a planned feature of populate in v2). You can however do the population from redux by hand since the data is available.

That means that data needed for populating can be gathered with populates in your query config, but you will need to do the mapping yourself.

Something like this if using v1.*.*:

const populates = [{ child: 'owner', root: 'users' }]

@firebaseConnect([
  { path: 'todos', populates}
])
@connect(
  (state => ({
    todos: dataToJS(state.firebase, 'todos'),
    populatedTodos: state.firebase.getIn(['data', 'todos'])
      ? state.firebase
          .getIn(['data', 'todos'])
          .map(todo => ({
            ...todo.toJS(),
            // get owner from users in redux defaulting to id if user not found
            owner: dataToJS(firebase, `users/${todo.get('owner')}`, todo.owner) 
          }))
          .toJS()
      : [],
  })
)

Or like this with v2.*.*:

const populates = [{ child: 'owner', root: 'users' }]

@firebaseConnect([
  { path: 'todos', populates }
])
@connect(
  (({ firebase: { ordered: { todos }, data: { users } }) => ({
    todos,
    populatedTodos: todos
      ? todos.map(todo => ({ ...todo, owner: users[todo.owner] || todo.owner }))
      : [],
  })
)

Obviously this is not as clean as directly using populatedDataToJS in v1.*.* or populate in v2.*.*, but as mentioned, it will be coming.

Always open to pull requests if you end up looking into the source.

@prescottprue prescottprue changed the title from Ordered populated data to feat(populate): populate ordered data Aug 16, 2017

@prescottprue prescottprue added this to the v2.0.0 milestone Aug 16, 2017

@benoj

This comment has been minimized.

Show comment
Hide comment
@benoj

benoj Aug 16, 2017

Thanks, will look to migrate to v 2.0.0

Just a further general question w.r.t ordered data. My data looks as below

comments:
 -KrcAcNlMnoEIGK7vYy5
     author: "30bp2ABAAwXtXR9zovUNTnz3uS62"
     text: "bing"
     timestamp: 1502842062097
 -KreuqJMGaXxIKZiwJUz
     author: "30bp2ABAAwXtXR9zovUNTnz3uS62"
     text: "bong"
     timestamp: 1502884879319

And my query looks like


const populates = [
{ child: 'author', root: `users` }
]

@firebaseConnect((props) => {
  const {currentOrg, candidateId} = props
  return [
    { path: `/comments`, populates, queryParams: ['orderByChild=timestamp']}
  ]
})

The comments are coming back as an object, is there a better way to query this so that they are coming back as an ordered list?

benoj commented Aug 16, 2017

Thanks, will look to migrate to v 2.0.0

Just a further general question w.r.t ordered data. My data looks as below

comments:
 -KrcAcNlMnoEIGK7vYy5
     author: "30bp2ABAAwXtXR9zovUNTnz3uS62"
     text: "bing"
     timestamp: 1502842062097
 -KreuqJMGaXxIKZiwJUz
     author: "30bp2ABAAwXtXR9zovUNTnz3uS62"
     text: "bong"
     timestamp: 1502884879319

And my query looks like


const populates = [
{ child: 'author', root: `users` }
]

@firebaseConnect((props) => {
  const {currentOrg, candidateId} = props
  return [
    { path: `/comments`, populates, queryParams: ['orderByChild=timestamp']}
  ]
})

The comments are coming back as an object, is there a better way to query this so that they are coming back as an ordered list?

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 16, 2017

Owner

The query is no different, you just get it from a different place in redux (i.e. ordered instead of `data). If you are currently using v1, then the following should work:

@connect(
  (({ firebase }) => ({
    todos: dataToJS(firebase, 'todos'),
    orderedTodos: orderedToJS(firebase, 'todos')
  })
)
Owner

prescottprue commented Aug 16, 2017

The query is no different, you just get it from a different place in redux (i.e. ordered instead of `data). If you are currently using v1, then the following should work:

@connect(
  (({ firebase }) => ({
    todos: dataToJS(firebase, 'todos'),
    orderedTodos: orderedToJS(firebase, 'todos')
  })
)
@benoj

This comment has been minimized.

Show comment
Hide comment
@benoj

benoj Aug 16, 2017

I meant more along the lines of my render method looks like this:

Object.keys(comments).map( (commentId) => {
              const { text, timestamp, author: { displayName, avatarUrl}} = comments[commentId]
              // render comment
})

But my understanding is that Object.keys does not necessarily guarantee the order.. is there a better solution to this using the orderedToJS?

benoj commented Aug 16, 2017

I meant more along the lines of my render method looks like this:

Object.keys(comments).map( (commentId) => {
              const { text, timestamp, author: { displayName, avatarUrl}} = comments[commentId]
              // render comment
})

But my understanding is that Object.keys does not necessarily guarantee the order.. is there a better solution to this using the orderedToJS?

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 16, 2017

Owner

@benoj orderedToJS is the same as dataToJS except for the fact that it does guarantee the order since it is an array, so not sure what is meant by "better".

Is there a reason you can't just pass the ordered data as your prop (as indicated in my last example)? That way, when you are mapping you can just to this.props.comments.map(someMapFunc) since ordered data is an Array instead of using Object.keys as needed when mapping.

As mentioned, a way to populate ordered data in v1

Owner

prescottprue commented Aug 16, 2017

@benoj orderedToJS is the same as dataToJS except for the fact that it does guarantee the order since it is an array, so not sure what is meant by "better".

Is there a reason you can't just pass the ordered data as your prop (as indicated in my last example)? That way, when you are mapping you can just to this.props.comments.map(someMapFunc) since ordered data is an Array instead of using Object.keys as needed when mapping.

As mentioned, a way to populate ordered data in v1

@benoj

This comment has been minimized.

Show comment
Hide comment
@benoj

benoj Aug 16, 2017

I am not seeing an array come through..

With the following I am seeing the ordered never coming through.. Am i misunderstanding how to use it? (Still on v1.5)

const populates = [
{ child: 'author', root: `users` }
]

@firebaseConnect(() => {
  return [
    { path: `/comments`, populates, queryParams: ['orderByChild=timestamp']}
  ]
})
@connect(({ firebase }) => {
  const comments = dataToJS(firebase, `/comments`)
  const ordered = orderedToJS(firebase,`/comments`)
  return {
    comments: isLoaded(comments) ? comments || {} : {},
    ordered: isLoaded(ordered) ? ordered || [] : [],
  }
})

benoj commented Aug 16, 2017

I am not seeing an array come through..

With the following I am seeing the ordered never coming through.. Am i misunderstanding how to use it? (Still on v1.5)

const populates = [
{ child: 'author', root: `users` }
]

@firebaseConnect(() => {
  return [
    { path: `/comments`, populates, queryParams: ['orderByChild=timestamp']}
  ]
})
@connect(({ firebase }) => {
  const comments = dataToJS(firebase, `/comments`)
  const ordered = orderedToJS(firebase,`/comments`)
  return {
    comments: isLoaded(comments) ? comments || {} : {},
    ordered: isLoaded(ordered) ? ordered || [] : [],
  }
})
@benoj

This comment has been minimized.

Show comment
Hide comment
@benoj

benoj Aug 16, 2017

^ here the comments is an object and ordered is coming through as the default []

benoj commented Aug 16, 2017

^ here the comments is an object and ordered is coming through as the default []

@neospirit

This comment has been minimized.

Show comment
Hide comment
@neospirit

neospirit Aug 17, 2017

Hi there,
I'm having some issues to get objects of authenticated user. For the moment, I have to hardcode the Firebase user ID in the "equalTo" query parameter. I think I miss something here but I'm close :) . I'm on the 2.0.0-beta6 by the way.

const populates = [ { orderByChild: 'createdAt' } ]

@UserIsAuthenticated
@firebaseConnect(() => [{
   path: '/todos', 
   populates, 
   queryParams: [
      'orderByChild=owner',
      'equalTo=MYFIREBASEUSERID'
    ]
  }
])

@connect(({ firebase: { auth, data: { todos } } }, { params }) => ({
  auth,
  todos
}))

neospirit commented Aug 17, 2017

Hi there,
I'm having some issues to get objects of authenticated user. For the moment, I have to hardcode the Firebase user ID in the "equalTo" query parameter. I think I miss something here but I'm close :) . I'm on the 2.0.0-beta6 by the way.

const populates = [ { orderByChild: 'createdAt' } ]

@UserIsAuthenticated
@firebaseConnect(() => [{
   path: '/todos', 
   populates, 
   queryParams: [
      'orderByChild=owner',
      'equalTo=MYFIREBASEUSERID'
    ]
  }
])

@connect(({ firebase: { auth, data: { todos } } }, { params }) => ({
  auth,
  todos
}))
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 17, 2017

Owner

@neospirit What you are asking about seems to be unrelated to this issue. Feel free to reach out over gitter or open a new issue if you believe you are experiencing a bug.

@benoj Usually I use isLoaded within the component's render itself so I can do different stuff (i.e. show a loader) when data is loading. From the looks of it though, what you are doing should work. Have you checked redux-devtools so you can see how the state looks?

Owner

prescottprue commented Aug 17, 2017

@neospirit What you are asking about seems to be unrelated to this issue. Feel free to reach out over gitter or open a new issue if you believe you are experiencing a bug.

@benoj Usually I use isLoaded within the component's render itself so I can do different stuff (i.e. show a loader) when data is loading. From the looks of it though, what you are doing should work. Have you checked redux-devtools so you can see how the state looks?

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 29, 2017

Owner

Initial support for populating ordered data has been added to the v2.0.0-beta.8 branch. Unit tests still need to be added.

It can be tested locally before the release with npm linking.

All it takes is placing ordered/ on the front of your populate call like so:

connect(({ firebase }) => ({
  todos: populate(firebase, 'todos', populates),
  orderedTodos: populate(firebase, 'ordered/todos', populates)
})

This update also comes with a ton of simplification to the populate logic so expanding it in the future should be even easier now.

Owner

prescottprue commented Aug 29, 2017

Initial support for populating ordered data has been added to the v2.0.0-beta.8 branch. Unit tests still need to be added.

It can be tested locally before the release with npm linking.

All it takes is placing ordered/ on the front of your populate call like so:

connect(({ firebase }) => ({
  todos: populate(firebase, 'todos', populates),
  orderedTodos: populate(firebase, 'ordered/todos', populates)
})

This update also comes with a ton of simplification to the populate logic so expanding it in the future should be even easier now.

@prescottprue prescottprue referenced this issue Sep 1, 2017

Merged

v2.0.0-beta.8 #263

3 of 3 tasks complete

prescottprue added a commit that referenced this issue Sep 2, 2017

v2.0.0-beta.8 (#263)
* fix(reducer): `MERGE` action added and `SET` action to reverted to v1 behavior - #255
* feat(populate): population of ordered data - #239
* feat(populate) populate once queries - #256
* feat(auth): emptyOnLogin config option added (defaults to `true`) - #254
* feat(query): emit `NO_VALUE` for `once` queries that are empty - #265
* fix(populate) Fixed populate function to return null for null paths
* app instance handling now uses `firebase_` instead of `extendApp` (more functionality) - #250
* Removed no longer in use code from v1 (validateConfig)
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Sep 2, 2017

Owner

Initial swing released in v2.0.0-beta.8. It should also work for once queries now too.

Reach out if things don't work as expected.

Owner

prescottprue commented Sep 2, 2017

Initial swing released in v2.0.0-beta.8. It should also work for once queries now too.

Reach out if things don't work as expected.

@minusoneman

This comment has been minimized.

Show comment
Hide comment
@minusoneman

minusoneman Sep 19, 2017

I've been testing the new populate method (v2.0.0-beta.8), but I think there is an issue with ordered data.

  orderedTodos: populate(firebase, 'ordered/todos', populates)

seems to return a (unpopulated) object - I would expect an array.

I've prepared a few failing unit tests, but I want to make sure I understand the expected behaviour before I start debugging the new populate method.

Let me know if I'm missing something - and thank you for a great library.

minusoneman commented Sep 19, 2017

I've been testing the new populate method (v2.0.0-beta.8), but I think there is an issue with ordered data.

  orderedTodos: populate(firebase, 'ordered/todos', populates)

seems to return a (unpopulated) object - I would expect an array.

I've prepared a few failing unit tests, but I want to make sure I understand the expected behaviour before I start debugging the new populate method.

Let me know if I'm missing something - and thank you for a great library.

@0x80

This comment has been minimized.

Show comment
Hide comment
@0x80

0x80 Nov 4, 2017

Contributor

Hi all, @prescottprue, I am trying to populate ordered data with beta.14, but I can't figure it out. In the code below the state contains firebase.ordered.approvals without the user being populated.

export const Pending: React.SFC<IPendingProps> = ({ data }) => {
  if (!data) return null;

  console.log('data', data);
  return <div>Pending approvals count: {data.length}</div>;
};

const populates = [{ child: 'user', root: 'users' }];

const fbWrapped = firebaseConnect([
  {
    path: 'approvals',
    queryParams: ['orderByChild=status', 'equalTo=pending'],
    populates,
  },
])(Pending);

export default connect(({ firebase }) => ({
  data: populate(firebase, 'ordered/approvals', populates, null),
  // data: populate(firebase, 'approvals', populates, null),
}))(fbWrapped);

The commented out version in connect works. So non-ordered data populates as expected. Am I missing something here?

Contributor

0x80 commented Nov 4, 2017

Hi all, @prescottprue, I am trying to populate ordered data with beta.14, but I can't figure it out. In the code below the state contains firebase.ordered.approvals without the user being populated.

export const Pending: React.SFC<IPendingProps> = ({ data }) => {
  if (!data) return null;

  console.log('data', data);
  return <div>Pending approvals count: {data.length}</div>;
};

const populates = [{ child: 'user', root: 'users' }];

const fbWrapped = firebaseConnect([
  {
    path: 'approvals',
    queryParams: ['orderByChild=status', 'equalTo=pending'],
    populates,
  },
])(Pending);

export default connect(({ firebase }) => ({
  data: populate(firebase, 'ordered/approvals', populates, null),
  // data: populate(firebase, 'approvals', populates, null),
}))(fbWrapped);

The commented out version in connect works. So non-ordered data populates as expected. Am I missing something here?

@prescottprue prescottprue reopened this Nov 5, 2017

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Nov 5, 2017

Owner

@0x80 The example you provided should be working. Reopened this to track looking into it.

Owner

prescottprue commented Nov 5, 2017

@0x80 The example you provided should be working. Reopened this to track looking into it.

@0x80

This comment has been minimized.

Show comment
Hide comment
@0x80

0x80 Nov 5, 2017

Contributor

I had a closer look the populate function with a debugger, and I think I spotted the problem.

When using a path in ordered, the value of data will be an array instead of an object. Therefor the last line below results in false, because lodash has() is not made for querying arrays.

// Gather data from top level if path is profile (handles populating profile)
  const data = get(state, dotPath, notSetValue)

 //.....

  // check for if data is single object or a list of objects
  const populatesForData = getPopulateObjs(
    isFunction(populates)
      ? populates(last(pathArr), data)
      : populates
  )
  // check each populate child parameter for existence
  const dataHasPopluateChilds = every(populatesForData, p => has(data, p.child))
Contributor

0x80 commented Nov 5, 2017

I had a closer look the populate function with a debugger, and I think I spotted the problem.

When using a path in ordered, the value of data will be an array instead of an object. Therefor the last line below results in false, because lodash has() is not made for querying arrays.

// Gather data from top level if path is profile (handles populating profile)
  const data = get(state, dotPath, notSetValue)

 //.....

  // check for if data is single object or a list of objects
  const populatesForData = getPopulateObjs(
    isFunction(populates)
      ? populates(last(pathArr), data)
      : populates
  )
  // check each populate child parameter for existence
  const dataHasPopluateChilds = every(populatesForData, p => has(data, p.child))
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Nov 5, 2017

Owner

@0x80 Great find! Totally open to a PR if you get the time, if not I will try to get to it when I can. Been switching my company over to v2 recently, so I have been super busy.

Owner

prescottprue commented Nov 5, 2017

@0x80 Great find! Totally open to a PR if you get the time, if not I will try to get to it when I can. Been switching my company over to v2 recently, so I have been super busy.

@0x80

This comment has been minimized.

Show comment
Hide comment
@0x80

0x80 Nov 5, 2017

Contributor
Contributor

0x80 commented Nov 5, 2017

@0x80

This comment has been minimized.

Show comment
Hide comment
@0x80

0x80 Nov 5, 2017

Contributor

BTW why is there a 2.0 branch and a 2.0 beta branch?

If 2.0 is still in beta I'd expect that to be the "next" branch or something, and the beta releases being tags in that branch. Creating this PR I wasn't sure if I should base it on beta.15 or 2.0.0.

Contributor

0x80 commented Nov 5, 2017

BTW why is there a 2.0 branch and a 2.0 beta branch?

If 2.0 is still in beta I'd expect that to be the "next" branch or something, and the beta releases being tags in that branch. Creating this PR I wasn't sure if I should base it on beta.15 or 2.0.0.

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Nov 5, 2017

Owner

There are the two branches since active changes are going onto beta.15, then merged back into v2.0.0. Things were done this way since before there was more than just v1.*.* and v2.*.* (when v1.4.* and v1.5.* were both active).

Totally agree this is unclear though. I like the idea of have the branch matching the tag name, so going to look into doing that. Thanks for the suggestion.

Owner

prescottprue commented Nov 5, 2017

There are the two branches since active changes are going onto beta.15, then merged back into v2.0.0. Things were done this way since before there was more than just v1.*.* and v2.*.* (when v1.4.* and v1.5.* were both active).

Totally agree this is unclear though. I like the idea of have the branch matching the tag name, so going to look into doing that. Thanks for the suggestion.

@prescottprue prescottprue referenced this issue Nov 6, 2017

Merged

v2.0.0 beta.15 #326

2 of 3 tasks complete

prescottprue added a commit that referenced this issue Nov 6, 2017

v2.0.0-beta.15
* feat(auth): `UNLOAD_PROFILE` is dispatched when `login` action creator is called - #301
* feat(auth): `signInWithPhoneNumber` action creator added - #319
* fix(firebaseConnect): function passed receives consistent arguments regardless of lifecycle - #320
* fix(query): `queryId` now includes `storeAs` to fix issue with listeners - #294
* fix(populate): ordered (array data) correctly populating - #239 - thanks @0x80 
* fix(reducer): property removed from profile is removed from state - thanks @fej-snikduj 
* fix(docs): inconsistencies in redux-auth-wrapper example - #325
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Nov 9, 2017

Owner

v2.0.0-beta.15 includes a fix for this.

Thanks for the pull request @0x80!

Owner

prescottprue commented Nov 9, 2017

v2.0.0-beta.15 includes a fix for this.

Thanks for the pull request @0x80!

@0x80

This comment has been minimized.

Show comment
Hide comment
@0x80

0x80 Nov 9, 2017

Contributor

@prescottprue BTW I'm now in the process of moving to Firestore. I read somewhere that populate doesn't work for Firestore yet. Is that correct? Would it require a similar approach?

Contributor

0x80 commented Nov 9, 2017

@prescottprue BTW I'm now in the process of moving to Firestore. I read somewhere that populate doesn't work for Firestore yet. Is that correct? Would it require a similar approach?

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Nov 10, 2017

Owner

@0x80 That is the case. populate itself should work the same accept for needing to get passed state.firestore instead of state.firebase. As for they query side of things, firestoreConnect does not support the populates parameter on its query config (the way firebaseConnect does), so that will need to be completed.

Hoping to make some progress on it this weekend.

Owner

prescottprue commented Nov 10, 2017

@0x80 That is the case. populate itself should work the same accept for needing to get passed state.firestore instead of state.firebase. As for they query side of things, firestoreConnect does not support the populates parameter on its query config (the way firebaseConnect does), so that will need to be completed.

Hoping to make some progress on it this weekend.

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