Skip to content
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

Support for JSON:API format json. #65

Open
NullVoxPopuli opened this issue Dec 11, 2017 · 5 comments
Open

Support for JSON:API format json. #65

NullVoxPopuli opened this issue Dec 11, 2017 · 5 comments

Comments

@NullVoxPopuli
Copy link
Collaborator

NullVoxPopuli commented Dec 11, 2017

I'm having trouble getting this to work with json:api ( jsonapi.org ) format data.
I think it's mostly because of the id being nested in the data object.

// single resource 
{ data: { id: 1 } }

// multiple resources
{ data: [
  { id: 1 }, { id: 2 }
]}

I think redux-crud-store expects the id to be at the top level of the json document.

maybe the adapter pattern from ember-data could help?

@devvmh
Copy link
Owner

devvmh commented Dec 12, 2017

I think this is supposed to work for the multiple resources case but not for the single resource case

The default ApiClient.js, assuming no errors, returns fetch api's response.json(), which is exactly the json from the server

sagas.js will put an action of type FETCH_SUCCESS with "payload" being the response returned from ApiClient.js

  try {
    const response = yield call(apiClient[method], path, { params, data, fetchConfig })
    yield put({ meta, type: success, payload: response })
  } catch (error) {
    yield put({ meta, type: failure, payload: error, error: true })
  }

reducers.js in the FETCH_SUCCESS handler looks like this:

    case FETCH_SUCCESS:
      const data = {}
      const payload = ('data' in action.payload) ? action.payload.data : action.payload
      payload.forEach((record) => {
        data[record.id] = {
          fetchTime: action.meta.fetchTime,
          error: null,
          record
        }
      })
      return Object.assign({}, state, data)

Does your example multiple resources work or not?

I think I do see a bug in FETCH_ONE_SUCCESS, which isn't checking for a data envelope. I have to think about the best way to check for this though.

To really reproduce this, I need actual sample data I think.

@devvmh
Copy link
Owner

devvmh commented Dec 12, 2017

Hm, I think there's no great way to support data envelopes for single records. Because there's no great way to tell the difference between

{ data: { id: 1, title: 'test' } }

and

{ id: 1, title: 'test', data: 'some field defined on the serverside' }

i.e. my logic currently just checks 'data' in payload but that can't tell these two cases apart.

I think this has to live in the ApiClient. I'd be happy to host a directory in this repo of custom api clients for common use cases, and jsonapi.org could be one of them.

So please open a PR with a working ApiClient for jsonapi.org and I'll review + merge. Or if you can think of a clever way to solve this problem, a PR for that would be even better.

I can take a stab at writing the ApiClient for you if you want, but I'm assuming it's a lot easier for you to write it since you can replicate/verify yourself.

@devvmh
Copy link
Owner

devvmh commented Dec 12, 2017

Please also let me know if this isn't working for FETCH actions (i.e. multiple records). If that's not working, that's a big issue I need to solve

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Dec 12, 2017

just saw your other comments on #64 (comment) and #66 (comment)

I think the best way to handle is to manually configure at adapter or expose an api for parsing models out of the payload.

because, even though the primary data resides in data there can also be relational data in included.
Like, this example from the jsonapi.org homepage:

{
  "data": [{
    "type": "articles",
    "id": "1",
    "attributes": {
      "title": "JSON API paints my bikeshed!"
    },
    "relationships": {
      "author": {
        "data": { "type": "people", "id": "9" }
      },
      "comments": {
        "data": [
          { "type": "comments", "id": "5" },
          { "type": "comments", "id": "12" }
        ]
      }
    },
  }],
  "included": [{
    "type": "people",
    "id": "9",
    "attributes": {
      "first-name": "Dan",
      "last-name": "Gebhardt",
      "twitter": "dgeb"
    },
  }, {
    "type": "comments",
    "id": "5",
    "attributes": {
      "body": "First!"
    },
    "relationships": {
      "author": {
        "data": { "type": "people", "id": "2" }
      }
    },
  }, {
    "type": "comments",
    "id": "12",
    "attributes": {
      "body": "I like XML better"
    },
    "relationships": {
      "author": {
        "data": { "type": "people", "id": "9" }
      }
    },
  }]
}

I think handling the relationships, and other features of jsonapi (like https://github.com/emberjs/data does) may be out of scope for this project, but it would be nice to be able to handle any format/structure of json and utilize the model-caching that redux-crud-store provides

@devvmh
Copy link
Owner

devvmh commented Dec 12, 2017

Nice. Another approach would be to implement normalizr a la #19 . But i think i like your idea of passing a parser/adapter to the apiclient better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants