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): alias option to store populated data on another param #126

Closed
Domiii opened this Issue May 8, 2017 · 11 comments

Comments

Projects
2 participants
@Domiii

Domiii commented May 8, 2017

Love the populate feature. But it has two short-comings (one causing a much greater issue than the other):

  1. It actually replaces the id with the object at the given populate path, thus making it impossible (or very bothersome) to get the actual id of the object (because in Firebase, it appears to be common practice to NOT add the id to the actual object). That is a huge issue if you need the ID (which is a very common use-case). Or maybe I overlooked an easy way of obtaining that id?
  2. We cannot alias the populated data field. E.g. I add an Id suffix to all id fields in my data, for multiple reasons. So basically I have an object containing uid, and then uid gets replaced with the actual user object (which is NOT an id). I would prefer to be able to add another argument to the populates objects, such as: { child: 'uid', root: 'users', childAlias: 'user' }.

I'd argue that solving issue (2) more or less involves fixing issue (1), since you actually want to add another property to the resulting object, instead of overriding the id property (or replace the original id prop with it's object, and then add another prop with the id, e.g.: the user property gets replaced, and a _user_id property is added, holding the original id). Either way, I feel that the solution to issue (1) can easily integrate an aliasing feature <3 <3 <3 :)

Hoping that at least issue (1) can be solved! (maybe the API already has a solution to this?) :)

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue May 8, 2017

Owner
  1. Option called keyProp (as seen in the the projects container of the material example)
const populates = [
  { child: 'uid', root: 'users', keyProp: 'uid' }  // keeps key on uid parameter
]
  1. Should be able to be solved with storeAs (as seen in the multiple queries example):
@firebaseConnect(({ auth }) => ([
  { path: 'projects', storeAs: 'myProjects' }
]))

It seems that both should be documented more clearly.

Owner

prescottprue commented May 8, 2017

  1. Option called keyProp (as seen in the the projects container of the material example)
const populates = [
  { child: 'uid', root: 'users', keyProp: 'uid' }  // keeps key on uid parameter
]
  1. Should be able to be solved with storeAs (as seen in the multiple queries example):
@firebaseConnect(({ auth }) => ([
  { path: 'projects', storeAs: 'myProjects' }
]))

It seems that both should be documented more clearly.

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue May 8, 2017

Owner

Let me know if this doesn't solve it for you, and I can reopen.

Owner

prescottprue commented May 8, 2017

Let me know if this doesn't solve it for you, and I can reopen.

@Domiii

This comment has been minimized.

Show comment
Hide comment
@Domiii

Domiii May 8, 2017

I'll open another one to ask for keyProp to be added to the relevant docs :D

Domiii commented May 8, 2017

I'll open another one to ask for keyProp to be added to the relevant docs :D

@Domiii

This comment has been minimized.

Show comment
Hide comment
@Domiii

Domiii May 8, 2017

  1. Problem solved! ()
  2. Sorry - I'm not quite sure how I can use storeAs to fix my problem? (X)

Right now, I have this in my query:

populates: [
  { child: 'uid', root: 'users', keyProp: 'uid' }
]

And here is what I have to do with my result object (which is called submission):

    let { 
      submission: {
        uid,
        ...
      }
    } = this.props;

    // the weird names are due to `populate` not allowing aliasing the populated object's key
    const user = uid;
    uid = user.uid;

Because populate replaced uid with the actual user object, to which it then added a uid property. However user should not replace uid. This is a problem of naming convention. I have the convention to suffix all my ids correspondingly.

Any way to get the naming fixed?

Domiii commented May 8, 2017

  1. Problem solved! ()
  2. Sorry - I'm not quite sure how I can use storeAs to fix my problem? (X)

Right now, I have this in my query:

populates: [
  { child: 'uid', root: 'users', keyProp: 'uid' }
]

And here is what I have to do with my result object (which is called submission):

    let { 
      submission: {
        uid,
        ...
      }
    } = this.props;

    // the weird names are due to `populate` not allowing aliasing the populated object's key
    const user = uid;
    uid = user.uid;

Because populate replaced uid with the actual user object, to which it then added a uid property. However user should not replace uid. This is a problem of naming convention. I have the convention to suffix all my ids correspondingly.

Any way to get the naming fixed?

@prescottprue prescottprue reopened this May 8, 2017

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue May 8, 2017

Owner

I misunderstood your original description. If you use keyProp: 'userId' or similar, then the ID would be placed under a different parameter, which may help for clarity.

That said, the renaming or "childAlias" that you are describing is not yet a feature, but I do like the idea of it.

Will most likely be placing on the roadmap for a coming version (still wrapping up v1.4.0, which is already feature complete).

Owner

prescottprue commented May 8, 2017

I misunderstood your original description. If you use keyProp: 'userId' or similar, then the ID would be placed under a different parameter, which may help for clarity.

That said, the renaming or "childAlias" that you are describing is not yet a feature, but I do like the idea of it.

Will most likely be placing on the roadmap for a coming version (still wrapping up v1.4.0, which is already feature complete).

@prescottprue prescottprue changed the title from Cannot get id when using populate to feat(populate): alias option to store populated data on another param May 8, 2017

@Domiii

This comment has been minimized.

Show comment
Hide comment
@Domiii

Domiii May 9, 2017

Thanks! Sounds great!

One more thing to consider: child (and now, also childAlias) and childParam props have ambiguous meanings of child: child refers to the id and the object we want to look up, while childParam refers to a child path inside the looked up object. Maybe want to clean up the naming before adding more ambiguity to the term child?

Maybe rename child to target and childAlias to targetAlias and childParam to targetChildPath or something like that (while of course still allowing the current naming convention, but phasing it out over the next few versions and show a warning message when using the old naming convention)?

Domiii commented May 9, 2017

Thanks! Sounds great!

One more thing to consider: child (and now, also childAlias) and childParam props have ambiguous meanings of child: child refers to the id and the object we want to look up, while childParam refers to a child path inside the looked up object. Maybe want to clean up the naming before adding more ambiguity to the term child?

Maybe rename child to target and childAlias to targetAlias and childParam to targetChildPath or something like that (while of course still allowing the current naming convention, but phasing it out over the next few versions and show a warning message when using the old naming convention)?

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue May 9, 2017

Owner

@Domiii That is a great suggestion for naming fixing clarity. Going to look into it.

Owner

prescottprue commented May 9, 2017

@Domiii That is a great suggestion for naming fixing clarity. Going to look into it.

@prescottprue prescottprue added this to the v1.5.0 milestone May 16, 2017

@prescottprue prescottprue modified the milestones: v1.6.0, v1.5.0 Aug 9, 2017

@prescottprue prescottprue self-assigned this Aug 9, 2017

prescottprue added a commit that referenced this issue Aug 30, 2017

Roadmap and README updated.
* #212 added to roadmap
* #126 added to roadmap
* Readme updated with messaging

@prescottprue prescottprue modified the milestones: v2.0.0, v1.6.0 Sep 2, 2017

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Sep 2, 2017

Owner

@Domiii Are you currently using v1 or v2?

There has been a ton of adoption of v2, so I was planning on moving this feature to there (meaning it will most likely get done sooner too). Any objections?

Owner

prescottprue commented Sep 2, 2017

@Domiii Are you currently using v1 or v2?

There has been a ton of adoption of v2, so I was planning on moving this feature to there (meaning it will most likely get done sooner too). Any objections?

@Domiii

This comment has been minimized.

Show comment
Hide comment
@Domiii

Domiii Sep 2, 2017

Sure!

TBH: I'm not sufficiently up2date on recent developments anyway, so I'm not sure what the trade-offs are... Sorry! :)

Domiii commented Sep 2, 2017

Sure!

TBH: I'm not sufficiently up2date on recent developments anyway, so I'm not sure what the trade-offs are... Sorry! :)

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Oct 13, 2017

Owner

Bump. Had a few more people ask for this, both for v1.*.* and v2.*.*. Going to move it up on the roadmap since it seems like so many people want it

Owner

prescottprue commented Oct 13, 2017

Bump. Had a few more people ask for this, both for v1.*.* and v2.*.*. Going to move it up on the roadmap since it seems like so many people want it

@prescottprue prescottprue added this to To Do in v2.0.0 Oct 18, 2017

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

Start of childAlias feature for populate - #126. Removed redux-firest…
…ore from import/export (it would need to be a dependency)

* Fix injectTapEventPlugin warning in material example
* Add component utils to material example

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

childAlias feature working - #126
* Option for including progress in file upload

@prescottprue prescottprue removed the question label Nov 27, 2017

@prescottprue prescottprue referenced this issue Nov 27, 2017

Merged

V2.0.0 beta.18 #345

3 of 3 tasks complete

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

v2.0.0-beta.18 (#345)
* feat(populate): `childAlias` for store results of populate on another parameter - #126
* feat(profile): Firestore support for `updateProfile` method - [issue 25 on redux-firestore](prescottprue/redux-firestore#25)
* feat(storage): `progress` option added to `uploadFile` method - #346
* feat(storage): `uploadFile` default metadata written to DB now includes `createdAt`
* feat(core): `redux-firestore` is no longer a dependency - managed by library user directly
* fix(examples): Material-ui example updates (including moving `injectTapEventPlugin()` to `main.js`)
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Dec 4, 2017

Owner

childAlias was added as part of the v2.0.0-beta.18 release. Thanks for the feature suggestion!

Owner

prescottprue commented Dec 4, 2017

childAlias was added as part of the v2.0.0-beta.18 release. Thanks for the feature suggestion!

v2.0.0 automation moved this from To Do to Done Dec 4, 2017

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