update buildURLForHasMany hook #2435

arenoir · 2014-10-29T22:51:11Z

Implement #2168 buildURLForHasMany hook on current build. It uses an explicit buildURL option rather than depending on links and ids being nil. I'm with @tomdale that a dsl is needed but I have needed/wanted this feature from day one.

@igorT let me know what you think #2162.

arenoir · 2014-10-31T19:45:43Z

@igorT do you have time to check this out? What do you think of adding options to the hasMany definition explicitly setting up how the relationship is resolved (ids, links, primaryKey/foreignKey)?

fivetanley · 2014-11-20T04:44:16Z

@arenoir thanks! sorry for the long delay. I'll ask @igorT to look at it, or we can bring it up at the next ember data meeting.

igorT · 2014-12-12T20:44:02Z

Crap, not sure how I missed this, will review asap

arenoir · 2014-12-15T19:44:20Z

@igorT yeah no worries. This is something that I am surprised is not a more sought after feature.

arenoir · 2015-01-04T21:47:14Z

@igorT any word on this? Is there anything I can do to help this along?

igorT · 2015-01-06T05:39:48Z

@arenoir holidays and crap came in between, reviewing now. Feel bad about leaving this hanging, happy to pair with you/screenshare on this or any other ED/Ember issue as a penance.

igorT · 2015-01-06T05:42:38Z

packages/ember-data/lib/adapters/rest_adapter.js

@@ -549,6 +549,18 @@ export default Adapter.extend({
    return url;
  },

+
+
+  buildURLForHasMany: function(relationshipName, type, record) {


I am not sure we want provide a default implementation, seems more like a fallback option?

igorT · 2015-01-06T05:49:46Z

I feel like instead of passing buildURL as an option, we should fallback to it if t is defined and the the server did not provide us with ids or with a url.

igorT · 2015-01-06T05:51:02Z

packages/ember-data/lib/system/relationships/state/has_many.js

+    this.updateLink(data.links[key]);
+  }
+
+  if (this.buildURL) {


Basically instead of the option, if no value and no link, we should set a flag that is 'generateLink' if there is a buildURLForHasMany in the adapter

We can then check the flag at https://github.com/arenoir/data/blob/build-hasmany-url/packages/ember-data/lib/system/relationships/state/has_many.js#L188 instead of relying on there being a link

And findLink can then generate a link if one is not provided

igorT · 2015-01-06T05:53:32Z

Again, apologies for taking this long, PRs like these are a bit scary and I suck :(

arenoir · 2015-01-06T19:13:48Z

@igorT you don't suck :) this is a overwhelming project. I am happy to be able to contribute.

I will make the changes you noted and rebase.

arenoir · 2015-01-06T21:19:52Z

@igorT take a look when you get some time. The biggest change was moving the buildUrlForHasMany function to the relationshipFromMeta function. I think it would be beneficial to add the same functionality to belongsTo relationships buildUrlForBelongsTo. This would be useful when dealing with composite primary keys.

igorT · 2015-01-06T21:22:01Z

packages/ember-data/lib/system/relationship-meta.js

@@ -16,13 +16,24 @@ export function typeForRelationshipMeta(store, meta) {
  return type;
 }

+export function generateLink(store, kind, type) {


Why not put it on the relationship class?

I may be misunderstanding the use but I was thinking If i put here the function would be looked up only once when the relationship is defined rater than each time the record is updated.

Not sure this makes a difference? You will call the function every time you want to generate a link no?

yes if the generateLink function is found it will call the returned function every time but if the function is not found it is only called once.

igorT · 2015-01-06T21:26:00Z

buildUrlForBelongsTo seems like a right thing to do as well

arenoir · 2015-01-06T21:40:35Z

okay cool I will add the buildUrlForBelongsTo and take care of the other notes.

arenoir · 2015-01-07T00:37:46Z

@igorT okay I took care of the issues you mentioned. My tests pass, It looks like Travis ci is crapping out when running npm.

arenoir · 2015-01-07T19:21:24Z

@igorT take a look when you get some time. I think this is ready to go.

Myztiq · 2015-01-14T18:31:33Z

+1 this is going to hopefully solve one of the biggest hurdles we have when using ember-data IMHO.

arenoir · 2015-03-23T19:53:14Z

@bmac, @igorT, @fivetanley just brought this up to date with master. Has this pr been discussed? What is preventing it from being merged? Is there anything I can do?

wecc · 2015-03-24T07:39:23Z

Is my understanding correct that this PR and #2898 are solving the same problem, but in different ways?

arenoir · 2015-03-24T17:17:13Z

Implementing RFC #4 looks like it would solve this problem. Will it land in 1.0? Until then I will keep this up to date.

arenoir · 2015-03-26T22:15:12Z

@wecc I believe PR #2898 and RFC #4 make the buildUrl more customizable but still require relationship payload from the server.

This pr adds extra functionality to generate the hasMany link automatically using the type and foreign_key.

bmac · 2015-06-05T10:24:04Z

This pr looks good to me. 👍 It just needs a rebase and a review from @igorT.

arenoir · 2015-06-05T16:26:07Z

I will rebase it today.

update restadapters buildURLForHasMany to use buildUrl merge upstream add run to failing test resolved conflicts update based on igor's comments check for type in generateLink update from master move generateLink into relationship class add execption to buildUrlForHasMany test fix context closure in generateLink function fix tests styling merge with master fix tests

arenoir · 2015-06-06T01:03:38Z

@bmac, I have rebased. However there are two tests that fail because of the way I restructured the setupRelationships function in packages/ember-data/lib/system/store.js line 2135.

The failing tests test the absence of new record relationships. And I am guessing record._relationships.get(key) lazily creates a relationship.

I like moving the logic of how to load the data into the relationship instance.

So I guess the question is why shouldn't new records have empty relationships?

@igorT any thoughts? Is this functionality that @tomdale originally implemented still sought after?

arenoir · 2015-07-13T06:51:50Z

Here is a work around.

https://gist.github.com/jamesarosen/79337a515f557d7e9cf6

greyhwndz · 2015-08-30T16:25:10Z

@igorT: Any update on this?

bmac · 2016-03-27T19:48:25Z

@arenoir any chance you have time to rebase this pr and put it behind a feature flag? If no let me know and I can do the work to rebase it.

bmac · 2016-06-24T20:21:18Z

@arenoir do you have time to rebase this pr and put it behind a feature flag?

arenoir · 2016-06-28T11:23:25Z

@bmac I don't know if this is necessary anymore? I am able to build the link for relationships within the models serializer normalize function. Let me know. I could possibly spend some time on this next week.

#/serializers/job.js
function urlForAttachments(jobId) {
  return `/api/v2/private_attachments?job_id=${jobId}`;
}

export default ApplicationSerializer.extend({
  normalize(typeClass, hash, prop) {
    hash.links = {
      privateAttachments: urlForAttachments(hash.id)
    };
    return this._super(typeClass, hash, prop);
  }
});

bmac · 2016-07-11T14:36:52Z

Thanks @arenoir. I'm willing to re-visit this in the future but for now I like the idea of building the link for relationships within the models serializer normalize function.

stefanpenner · 2017-03-08T07:53:42Z

This seems to have gone stale. If this is something we want, @bmac suggested it go behind a feature flag, if not we can leave this closed.

fivetanley added the team-review label Nov 20, 2014

igorT force-pushed the master branch from b38d12f to 894457a Compare November 26, 2014 18:59

fivetanley mentioned this pull request Dec 10, 2014

Ember Data Meeting PR Discussion 2014/12/10 #2573

Closed

10 tasks

fivetanley mentioned this pull request Dec 17, 2014

Ember Data Meeting PR Discussion 2014/12/17 #2606

Closed

5 tasks

arenoir force-pushed the build-hasmany-url branch from a5b8c5a to 5be1c1c Compare December 20, 2014 01:54

igorT reviewed Jan 6, 2015
View reviewed changes

igorT mentioned this pull request Jan 6, 2015

Introduce buildURLForHasMany hook #2168

Closed

arenoir force-pushed the build-hasmany-url branch from a3967b9 to 8677f0a Compare January 6, 2015 21:09

igorT reviewed Jan 6, 2015
View reviewed changes

arenoir force-pushed the build-hasmany-url branch from 8677f0a to 8501340 Compare January 6, 2015 23:15

arenoir force-pushed the build-hasmany-url branch from 351ed09 to 19be12b Compare January 7, 2015 18:57

arenoir force-pushed the build-hasmany-url branch 3 times, most recently from dd04ccf to ca0c098 Compare January 7, 2015 19:33

arenoir force-pushed the build-hasmany-url branch from 9d1cdeb to 743dc9d Compare January 25, 2015 19:30

bmac removed the team-review label Feb 13, 2015

arenoir force-pushed the build-hasmany-url branch 2 times, most recently from 3f5e631 to 0cffbca Compare March 23, 2015 19:37

arenoir force-pushed the build-hasmany-url branch from 0cffbca to 0ad054c Compare March 26, 2015 22:03

arenoir force-pushed the build-hasmany-url branch from 4eaded8 to ca02f54 Compare June 6, 2015 00:50

arenoir mentioned this pull request Aug 25, 2015

Support query params when fetching hasMany relationship #3700

Closed

stefanpenner closed this Mar 8, 2017

update buildURLForHasMany hook #2435

update buildURLForHasMany hook #2435

Conversation

arenoir commented Oct 29, 2014

arenoir commented Oct 31, 2014

fivetanley commented Nov 20, 2014

igorT commented Dec 12, 2014

arenoir commented Dec 15, 2014

arenoir commented Jan 4, 2015

igorT commented Jan 6, 2015

Choose a reason for hiding this comment

igorT commented Jan 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorT commented Jan 6, 2015

arenoir commented Jan 6, 2015

arenoir commented Jan 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorT commented Jan 6, 2015

arenoir commented Jan 6, 2015

arenoir commented Jan 7, 2015

arenoir commented Jan 7, 2015

Myztiq commented Jan 14, 2015

arenoir commented Mar 23, 2015

wecc commented Mar 24, 2015

arenoir commented Mar 24, 2015

arenoir commented Mar 26, 2015

bmac commented Jun 5, 2015

arenoir commented Jun 5, 2015

arenoir commented Jun 6, 2015

arenoir commented Jul 13, 2015

greyhwndz commented Aug 30, 2015

bmac commented Mar 27, 2016

bmac commented Jun 24, 2016

arenoir commented Jun 28, 2016 • edited Loading

bmac commented Jul 11, 2016

stefanpenner commented Mar 8, 2017

arenoir commented Jun 28, 2016 •

edited

Loading