RFC for reference unification #57

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
10 participants
@wycats
Member

wycats commented May 23, 2015

@igorT

This comment has been minimized.

Show comment
Hide comment
@igorT

igorT May 24, 2015

Member

Several concerns that were raised in the Slack room:

  1. It seems like people would want to manipulate relationships based on just having the reference, and not having the full object. Is this supported behavior? Basically setting a belongsTo to a reference.

  2. It will be easy for references to end up in the dom, see for example:

    {{each post.comments as |comment|}}
         {{linkTo 'comment' comment.id}}
    {{/each}}
    

    If we want to, how do we stop this? How do we enable this use case?

  3. Lets say you want to filer all the comments whose id is not equal to 1. That CP is going to be annoying/hard to write, because you will need to be filtering on relationshipReferences, but the CP will not be able to observe those, it can only observe comments.@each.id or something like that.

Member

igorT commented May 24, 2015

Several concerns that were raised in the Slack room:

  1. It seems like people would want to manipulate relationships based on just having the reference, and not having the full object. Is this supported behavior? Basically setting a belongsTo to a reference.

  2. It will be easy for references to end up in the dom, see for example:

    {{each post.comments as |comment|}}
         {{linkTo 'comment' comment.id}}
    {{/each}}
    

    If we want to, how do we stop this? How do we enable this use case?

  3. Lets say you want to filer all the comments whose id is not equal to 1. That CP is going to be annoying/hard to write, because you will need to be filtering on relationshipReferences, but the CP will not be able to observe those, it can only observe comments.@each.id or something like that.

@igorT

This comment has been minimized.

Show comment
Hide comment
@igorT

igorT May 25, 2015

Member

emberjs/data#2264 seems relevant

Member

igorT commented May 25, 2015

emberjs/data#2264 seems relevant

@thomassnielsen

This comment has been minimized.

Show comment
Hide comment
@thomassnielsen

thomassnielsen May 25, 2015

I like this a whole lot, and it would solve many of the use cases where we today have unnecessary fetches against the server and/or usage of private APIs. I also agree that the drawbacks seems worth it as described.

What I'm missing currently is details on how (and if) this will affect current APIs. Especially what Igor describes in point 2, about references not leaking into record arrays (see emberjs/data#2375 for a similar issue). As I see it, the current APIs should maintain only showing loaded records, and references should only show up when using the new reference APIs. Any and all template usage should only show loaded records. I think it would be helpful if this is more explicitly stated in the RFC (if this is indeed the intent).

I also think what Igor describes in point 1 should be implemented if possible without increasing complexity too much. Being able to manipulate relationships based on reference would save a lot of API calls and increase performance in several apps I'm currently working on.

I like this a whole lot, and it would solve many of the use cases where we today have unnecessary fetches against the server and/or usage of private APIs. I also agree that the drawbacks seems worth it as described.

What I'm missing currently is details on how (and if) this will affect current APIs. Especially what Igor describes in point 2, about references not leaking into record arrays (see emberjs/data#2375 for a similar issue). As I see it, the current APIs should maintain only showing loaded records, and references should only show up when using the new reference APIs. Any and all template usage should only show loaded records. I think it would be helpful if this is more explicitly stated in the RFC (if this is indeed the intent).

I also think what Igor describes in point 1 should be implemented if possible without increasing complexity too much. Being able to manipulate relationships based on reference would save a lot of API calls and increase performance in several apps I'm currently working on.

@sandstrom

This comment has been minimized.

Show comment
Hide comment
@sandstrom

sandstrom May 25, 2015

Looks great! ⛵️ We have a few places where we need to determining if a reference exist without triggering a server-fetch. This would allows us to move off the private APIs we're currently using.


Not 100% related, but it would allow for a JSON-API adapter to sub-class RecordArray and add a loadNext method, utilizing pagination metadata coupled with the metadata on the reference, to retrieve, say, the next 100 comments for a post.

Looks great! ⛵️ We have a few places where we need to determining if a reference exist without triggering a server-fetch. This would allows us to move off the private APIs we're currently using.


Not 100% related, but it would allow for a JSON-API adapter to sub-class RecordArray and add a loadNext method, utilizing pagination metadata coupled with the metadata on the reference, to retrieve, say, the next 100 comments for a post.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats May 28, 2015

Member

It seems like people would want to manipulate relationships based on just having the reference, and not having the full object. Is this supported behavior? Basically setting a belongsTo to a reference.

The current proposal does not support mutation, but I would be happy to add it if people need it. Perhaps: reference.update({ ... }). I'm not sure if it makes sense to support updating everything (like id) but maybe it is?

It will be easy for references to end up in the dom, see for example:

I don't understand how your example puts references in the DOM. Can you give me the JS code that produces this?

How do we enable this use case?

Can you flesh out the use-case?

Lets say you want to filer all the comments whose id is not equal to 1. That CP is going to be annoying/hard to write, because you will need to be filtering on relationshipReferences, but the CP will not be able to observe those, it can only observe comments.@each.id or something like that

This proposal doesn't address that case. Do you think it's important to solve it now? If so, I can give it some thought.

Member

wycats commented May 28, 2015

It seems like people would want to manipulate relationships based on just having the reference, and not having the full object. Is this supported behavior? Basically setting a belongsTo to a reference.

The current proposal does not support mutation, but I would be happy to add it if people need it. Perhaps: reference.update({ ... }). I'm not sure if it makes sense to support updating everything (like id) but maybe it is?

It will be easy for references to end up in the dom, see for example:

I don't understand how your example puts references in the DOM. Can you give me the JS code that produces this?

How do we enable this use case?

Can you flesh out the use-case?

Lets say you want to filer all the comments whose id is not equal to 1. That CP is going to be annoying/hard to write, because you will need to be filtering on relationshipReferences, but the CP will not be able to observe those, it can only observe comments.@each.id or something like that

This proposal doesn't address that case. Do you think it's important to solve it now? If so, I can give it some thought.

@igorT

This comment has been minimized.

Show comment
Hide comment
@igorT

igorT May 28, 2015

Member
{{each post.commentReferences as |comment|}}
     {{linkTo 'comment' comment.id}}
{{/each}}
Member

igorT commented May 28, 2015

{{each post.commentReferences as |comment|}}
     {{linkTo 'comment' comment.id}}
{{/each}}
@fivetanley

This comment has been minimized.

Show comment
Hide comment
@fivetanley

fivetanley May 29, 2015

Member

More discussion on getting the ids in a template here as well: worth reviewing emberjs/data#3025

cc @chancancode

Member

fivetanley commented May 29, 2015

More discussion on getting the ids in a template here as well: worth reviewing emberjs/data#3025

cc @chancancode

@igorT igorT referenced this pull request in emberjs/data May 31, 2015

Closed

isReloading flag not set on hasMany relationships #3115

@igorT

This comment has been minimized.

Show comment
Hide comment
@igorT

igorT May 31, 2015

Member

Another use case is to want to bind to isLoading or isReloading of the relationship

Member

igorT commented May 31, 2015

Another use case is to want to bind to isLoading or isReloading of the relationship

@wecc

This comment has been minimized.

Show comment
Hide comment
@wecc

wecc May 31, 2015

Comment from @igorT:

@tomdale and @wycats are to write up a nice example on how to get IDs and how to use isLoaded

wecc commented May 31, 2015

Comment from @igorT:

@tomdale and @wycats are to write up a nice example on how to get IDs and how to use isLoaded

@pangratz pangratz referenced this pull request in emberjs/data Jun 11, 2015

Merged

Implement RFC 57 - Reference Unification #3303

@jdurand

This comment has been minimized.

Show comment
Hide comment
@jdurand

jdurand Jun 25, 2015

I used to do this the non-public (dirty) way with province.data.country_id. I upgraded to ember 1.13.2 and ember-data 1.13.4 and the foreign key ids are not accessible through model.data anymore.

I'd like to quick-fix this with a dirty (or not) solution.
Did anyone figure out how to do this without having to patch their API server?

jdurand commented Jun 25, 2015

I used to do this the non-public (dirty) way with province.data.country_id. I upgraded to ember 1.13.2 and ember-data 1.13.4 and the foreign key ids are not accessible through model.data anymore.

I'd like to quick-fix this with a dirty (or not) solution.
Did anyone figure out how to do this without having to patch their API server?

+*/
+```
+
+### `ids`

This comment has been minimized.

@fivetanley

fivetanley Jun 25, 2015

Member

i think this should be id as it is a belongsTo and will only have one id, thus not being plural ids

@fivetanley

fivetanley Jun 25, 2015

Member

i think this should be id as it is a belongsTo and will only have one id, thus not being plural ids

@fivetanley

This comment has been minimized.

Show comment
Hide comment
@fivetanley

fivetanley Jun 25, 2015

Member

I really like the direction this is headed. It seems like a lot of things that were not possible using public APIs are now possible in JavaScript land. I'm wondering what this means in templates though; it seems like it may be a bit cumbersome to get these references yourself every time and define them in a routable component.

API of my dreams:

{{#if comment.isLoaded}}
  your commentID is {{comment.id}}
{{else}}
  <button {{action "load-comment"}}>
    Load comment
  </button>
{{/if}}

With this current RFC, I don't think this is quite possible without always defining isLoaded, etc yourself in every component or every model.

This RFC introduces good primitives; my question for @wycats and @igorT is can we make some nice sugar on top of this in a semver compatible way.

Member

fivetanley commented Jun 25, 2015

I really like the direction this is headed. It seems like a lot of things that were not possible using public APIs are now possible in JavaScript land. I'm wondering what this means in templates though; it seems like it may be a bit cumbersome to get these references yourself every time and define them in a routable component.

API of my dreams:

{{#if comment.isLoaded}}
  your commentID is {{comment.id}}
{{else}}
  <button {{action "load-comment"}}>
    Load comment
  </button>
{{/if}}

With this current RFC, I don't think this is quite possible without always defining isLoaded, etc yourself in every component or every model.

This RFC introduces good primitives; my question for @wycats and @igorT is can we make some nice sugar on top of this in a semver compatible way.

@arenoir

This comment has been minimized.

Show comment
Hide comment
@arenoir

arenoir Jul 13, 2015

@jdurand I am also looking for a quick fix to accessing foreign keys. Did you have any success?

arenoir commented Jul 13, 2015

@jdurand I am also looking for a quick fix to accessing foreign keys. Did you have any success?

@jdurand

This comment has been minimized.

Show comment
Hide comment
@jdurand

jdurand Jul 13, 2015

@arenoir no, I haven't had time to dig into this yet.

jdurand commented Jul 13, 2015

@arenoir no, I haven't had time to dig into this yet.

@arenoir

This comment has been minimized.

Show comment
Hide comment
@arenoir

arenoir Jul 13, 2015

@jdurand okay here is my "quick fix". Added overrode the normalizeAttributes hook in the serializer to copy the foreign key. IMO the foreign_key should be public and not be thrown away.

//models/checklist-item.js
export default DS.Model.extend({
  timeEntry: belongsTo('time-entry', {async: true}),
  timeEntryId: attr('string'), //set in serializer
});

//serializers/checklist-item.js
export default DS.ActiveModelSerializer.extend({
  normalizeAttributes(typeClass, hash) {
    var foreignKey = hash['timeEntry'];

    if (foreignKey) {
      hash['timeEntryId'] = foreignKey;
    }

    return this._super(typeClass, hash);
  }
});

I think this a pretty standard use case. Hope this helps.

arenoir commented Jul 13, 2015

@jdurand okay here is my "quick fix". Added overrode the normalizeAttributes hook in the serializer to copy the foreign key. IMO the foreign_key should be public and not be thrown away.

//models/checklist-item.js
export default DS.Model.extend({
  timeEntry: belongsTo('time-entry', {async: true}),
  timeEntryId: attr('string'), //set in serializer
});

//serializers/checklist-item.js
export default DS.ActiveModelSerializer.extend({
  normalizeAttributes(typeClass, hash) {
    var foreignKey = hash['timeEntry'];

    if (foreignKey) {
      hash['timeEntryId'] = foreignKey;
    }

    return this._super(typeClass, hash);
  }
});

I think this a pretty standard use case. Hope this helps.

@sandstrom

This comment has been minimized.

Show comment
Hide comment
@sandstrom

sandstrom Aug 27, 2015

This would be a great addition to Ember Data! ⛵️

@wycats do you want to wait for more feedback, or is this RFC ready for activation?

This would be a great addition to Ember Data! ⛵️

@wycats do you want to wait for more feedback, or is this RFC ready for activation?

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Aug 27, 2015

Member

@sandstrom I'm happy with it, assuming it gets further iterated on over time to handle use-cases we missed in the initial design.

Member

wycats commented Aug 27, 2015

@sandstrom I'm happy with it, assuming it gets further iterated on over time to handle use-cases we missed in the initial design.

@sandstrom

This comment has been minimized.

Show comment
Hide comment
@sandstrom

sandstrom Aug 28, 2015

@wycats Sounds great! I've tried to compile what people have said above (please add if I've missed something).

  • Should mutation be added, or is reading enough for the first version?
  • References ending up in the DOM[1]. As I've understood it this won't happen with this API.
  • Use-case: "Filtering all comments whose id is not equal to 1". Should this be supported? Current proposal doesn't cover this.
  • How will isLoaded be handled? It was said above that Tom and Yehuda would write up an example. Is such an example still needed?

This would be very useful in our app, we're currently using private APIs[2]. I'd rather see a smaller implementation soon, than wait for the utopian RFC covering all use-cases. 😄

Carbon copy: @igorT @wecc @fivetanley


1. Leaking references into DOM

{{! will this leak references? I don't think it will, but please correct me if I'm wrong}}
{{each post.commentReferences as |comment|}}
  {{linkTo 'comment' comment.id}}
{{/each}}

2. Brittle Workaround

// brittle workaround
model._internalModel._relationships.initializedRelationships[relName].canonicalState.length

@wycats Sounds great! I've tried to compile what people have said above (please add if I've missed something).

  • Should mutation be added, or is reading enough for the first version?
  • References ending up in the DOM[1]. As I've understood it this won't happen with this API.
  • Use-case: "Filtering all comments whose id is not equal to 1". Should this be supported? Current proposal doesn't cover this.
  • How will isLoaded be handled? It was said above that Tom and Yehuda would write up an example. Is such an example still needed?

This would be very useful in our app, we're currently using private APIs[2]. I'd rather see a smaller implementation soon, than wait for the utopian RFC covering all use-cases. 😄

Carbon copy: @igorT @wecc @fivetanley


1. Leaking references into DOM

{{! will this leak references? I don't think it will, but please correct me if I'm wrong}}
{{each post.commentReferences as |comment|}}
  {{linkTo 'comment' comment.id}}
{{/each}}

2. Brittle Workaround

// brittle workaround
model._internalModel._relationships.initializedRelationships[relName].canonicalState.length

@wecc wecc referenced this pull request in emberjs/data Sep 13, 2015

Closed

Ability to reload hasMany conditionally #3740

@pangratz pangratz referenced this pull request in ember-data/active-model-adapter Oct 27, 2015

Closed

ID is not stored #55

@sandstrom

This comment has been minimized.

Show comment
Hide comment
@sandstrom

sandstrom Nov 2, 2015

Friendly ping 😄 @igorT @wecc @fivetanley @wycats

This is a great proposal that would solve a concrete pain in our app.

On the checklist above, what are your thoughts? (It's a list of all things mentioned in this thread, I think this proposal is ready for the next stage as soon as you guys feel that they are resolved; can add to the list if something is missing)

Friendly ping 😄 @igorT @wecc @fivetanley @wycats

This is a great proposal that would solve a concrete pain in our app.

On the checklist above, what are your thoughts? (It's a list of all things mentioned in this thread, I think this proposal is ready for the next stage as soon as you guys feel that they are resolved; can add to the list if something is missing)

@sandstrom

This comment has been minimized.

Show comment
Hide comment
@sandstrom

sandstrom Jan 11, 2016

Should this be closed now, since emberjs/data#3303 is merged?

Should this be closed now, since emberjs/data#3303 is merged?

@fivetanley

This comment has been minimized.

Show comment
Hide comment
@fivetanley

fivetanley Jan 11, 2016

Member

merged as 074c798. thanks for the reminder @sandstrom!

Member

fivetanley commented Jan 11, 2016

merged as 074c798. thanks for the reminder @sandstrom!

@fivetanley fivetanley closed this Jan 11, 2016

@fivetanley fivetanley deleted the ember-data-reference-unification branch Jan 11, 2016

@ef4

This comment has been minimized.

Show comment
Hide comment
@ef4

ef4 Mar 4, 2016

Contributor

FYI, I just edited the above original post to make "Rendered" be a valid permalink. It was broken because it pointed at a file under active on master.

Contributor

ef4 commented Mar 4, 2016

FYI, I just edited the above original post to make "Rendered" be a valid permalink. It was broken because it pointed at a file under active on master.

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