kurko / ember-json-api

JSON API adapter for Ember Data.
MIT License
254 stars 62 forks source link

Updated to the RC3 standard of JSON API #71

Closed eneuhauser closed 9 years ago

eneuhauser commented 9 years ago

Per JSON API RC3 release, the spec is close to being finalized. Now there is a line in the sand, it's easy to build against the RC3 standard until either another release candidate or 1.0 is ratified. The main feature that this pull request supports that #68 does not is the consistent linkage change, where a linked resource will have a linkage property that includes the type and id. The serializer is not backwards compatible and only works with the RC3 standard.

One concept of JSON API I have not been able to support is the self link. The current interface for the adapter does not allow accessing the owner record when doing a createRecord or updateRecord. I'm storing the self link within the resource's links, but there currently isn't a way to access it.

On the current project I'm working on, I have updated my server to be compliant with the RC3 standard and this adapter and serializer is working well.

eneuhauser commented 9 years ago

For those using my branch, please checkout my latest changes. I've merged the changes that have been checked into the master since my fork. One in the master and one into my branch improve polymorphic relationships, so hopefully that will resolve the major outstanding issues.

I believe my last set of changes already required Ember Data to 1.0 beta-16, but these latest changes definitely require an upgrade.

green-arrow commented 9 years ago

@eneuhauser I pulled down the latest changes, but still have issues with polymorphic relationships. Unfortunately, the model is not being pushed onto my store. In order to get that to work, I need the following extension of findBelongsTo in my adapter:

findBelongsTo: function(store, snapshot, url, relationship) {
  var promise = this._super(store, snapshot, url, relationship);

  if(promise) {
    promise = promise.then((response) => {
      if(relationship.options.polymorphic) {
        relationship.type = store.modelFor(response.data.type);
      }

      return response;
    });
  }

  return promise;
}
fotinakis commented 9 years ago

Now that compound documents will be fully supported, is there way to add query params for the include key when fetching a single record?

I was thinking something like:

model: function(params) {
  return this.store.find('post', params.id, {include: 'comments'})
}

...but this doesn't work, it seems the store interface doesn't support single-record query params at all yet and it's a 2-year old issue (https://github.com/emberjs/data/issues/1576).

Any workarounds come to mind? I think this will come up more and more as people start using JSON API more since include is a powerful feature there.

tobyzerner commented 9 years ago

@fotinakis Implement your own findQueryOne method, based on this PR https://github.com/emberjs/data/pull/2584

// initializers/find-query-one.js
import DS from 'ember-data';

export default {
  name: 'find-query-one',
  initialize: function(container) {
    DS.Store.reopen({
      findQueryOne: function(type, id, query) {
        var store = this;
        var typeClass = store.modelFor(type);
        var adapter = store.adapterFor(typeClass);
        var serializer = store.serializerFor(typeClass);
        var url = adapter.buildURL(type, id);
        var ajaxPromise = adapter.ajax(url, 'GET', { data: query });

        return ajaxPromise.then(function(rawPayload) {
          var extractedPayload = serializer.extract(store, typeClass, rawPayload, id, 'find');
          return store.push(typeClass, extractedPayload);
        });
      }
    });
  }
};
fotinakis commented 9 years ago

I'll check it out, thanks @tobscure.

green-arrow commented 9 years ago

@eneuhauser looks like the code I put in place for the findHasMany duplicate requests fix actually causes some issues. When executing model.reload() or model.get('relationship').reload(), that method is hit once again. However, since it was already previously loaded, no API call is executed.

I'll try to look into that some more this week, but I welcome any thoughts / ideas.

green-arrow commented 9 years ago

@kurko where do we stand on getting this merged? It would be nice to be able to get that done and then be able to open up separate issues for the things we run into (like the model reloading things I just mentioned above). Thanks!

fotinakis commented 9 years ago

Definite :+1: for getting this merged in and filing followup bugs, I have some small issues I've been holding on filing until this gets merged.

jamonholmgren commented 9 years ago

So far it's working well for me! :+1:

kurko commented 9 years ago

@green-arrow I'm dying to merge this, but I don't want bugs to be introduced in it. If the issues you have were present before, then it's fine. I don't have a RC3 app right now to test it myself, so I need you folks to tell me whether the pending issues are blocking or not.

@eneuhauser looks like the code I put in place for the findHasMany duplicate requests fix actually causes some issues

How do we stand with this?

green-arrow commented 9 years ago

@kurko the issue it presents is that the findMany relationship is never reloaded, even if model.get('relationship').reload() is executed. Since the relationship was loaded already, it never executes another API request.

I have not yet found a way to make this method re-execute the request if a reload has been requested.

eneuhauser commented 9 years ago

This conversation is getting huge! I have enabled issues within my repository to better track outstanding items. I have been wrapped up in a couple of projects, but I should have time soon to address reported issues.

fotinakis commented 9 years ago

@kurko we need you! :) You good with merging this?

gentle-noah commented 9 years ago

Thank you for all the work on this! This is exactly the solution I've been looking for, for the errors I've been running into :smile:

jonkoops commented 9 years ago

@eneuhauser Seems like issue #3 is a blocking issue in specification compliance.

andrewbranch commented 9 years ago

@jonkoops I submitted a PR to @eneuhauser implementing the PUT/PATCH swap needed for specification compliance: eneuhauser/ember-json-api#6

kurko commented 9 years ago

Ok, so unless someone comes shouting and saying this is not working, I'm merging this tonight and releasing a new version :smile:

fotinakis commented 9 years ago

:+1: :+1: :+1: Yay!

jamonholmgren commented 9 years ago

Awesome!

eneuhauser commented 9 years ago

Awesome, thanks! There are 3 issues reported in my repository that I haven't had a chance to look at yet. Additionally, I'm working on the findHasMany issue. I'm hoping to have time tomorrow to work on them. If you merge the current pull request, I can submit separate pull requests to solve those issues.

Edit: After looking at the issues, I don't think there is anything to fix. The findHasMany is the only open issue.

jamonholmgren commented 9 years ago

@eneuhauser Thanks for your hard work on this. And @kurko of course. As an open source project maintainer myself, I know these hours sometimes go unnoticed. Much appreciated.

jonkoops commented 9 years ago

I think we should get eneuhauser#8 fixed first, it's a minor issue but it could cause some confusion.

eneuhauser commented 9 years ago

I've made some comments in issue#8. Would love some feedback if I should merge my fix.

joelcox commented 9 years ago

Thanks for your great work on this @eneuhauser. We've been using your branch for some time now and I haven't run into any big issues so far.

jonkoops commented 9 years ago

@kurko In my opinion we should get this merged, @eneuhauser anything blocking in your opinion?

eneuhauser commented 9 years ago

@jonkoops and @kurko, I think RC3 is good to go. There will be more changes before 1.0 hits. Someone already opened Issue 10 about a change since RC3.

Since ember-json-api is one of the de facto implementations of JSON API, I think it would be a good idea to keep up with the release candidates. I can begin work on that issue, but until another release candidate hits or 1.0, I'm not going to merge it in.

Boubalou commented 9 years ago

@eneuhauser Based on the latest RC3 spec, there is an attributes field that hold all of the model fields, is there any plan on updating this PR to support the final revision RC3?

Edit: Nevermind this question, it is already covered in issue 10.

Keep it up!

green-arrow commented 9 years ago

:+1: on getting this merged

jonkoops commented 9 years ago

@kurko https://youtu.be/vCadcBR95oU

kurko commented 9 years ago

Folks, I'm merging this soon. I'm on vacation and without computer. Let me see if I can do something via mobile this weekend 😄 On Tue, May 12, 2015 at 7:09 PM Jon Koops notifications@github.com wrote:

@kurko https://github.com/kurko https://youtu.be/vCadcBR95oU

— Reply to this email directly or view it on GitHub https://github.com/kurko/ember-json-api/pull/71#issuecomment-101352784.

kurko commented 9 years ago

:smile:

Now, would someone take the torch for RC4? Seems pretty straightforward

eneuhauser commented 9 years ago

Thanks for merging! I have been heads down on a few non-Ember projects, but I am days away from a vacation. I should have some downtime to update to RC4. Someone has already submitted a pull request into my repo. I'll also look into upgrading Ember and Ember Data versions. According to the status page, JSON API 1.0 is slated for tomorrow. That will get some consistencies in implementations to get everyone on the same page.

josepjaume commented 9 years ago

For the record: The latest developments are in https://github.com/eneuhauser/ember-json-api/pull/14. I'm using it in my app and seems to work fine :+1: