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

Handling cases where jsonapi.version isn't set #378

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

krainboltgreene
Copy link

#291 is a problem for anyone using the ruby library jsonapi-resources, as it both doesn't support returning /jsonapi/version and it is very annoying to hack in.

I prefer to trust but warn, but failing that I've at least checked for a data attribute.

@ghost
Copy link

ghost commented Apr 30, 2019

There were the following issues with this Pull Request

  • Commit: 1069ff1
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

}
function isDocWithData(doc: any): doc is DocWithData {
return isJsonApi(doc) && ['object', 'array'].indexOf(typeOf((doc as DocWithData).data)) >= 0;
function isValidResponse(doc: any): doc is DocWithData {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I don't understand typescript, but why isn't the return type boolean?

@krainboltgreene
Copy link
Author

What is commitlint talking about here?

@ghost
Copy link

ghost commented Apr 30, 2019

There were the following issues with this Pull Request

  • Commit: 1069ff1
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: d1c85c9
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented Apr 30, 2019

There were the following issues with this Pull Request

  • Commit: 1069ff1
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: d1c85c9
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: e44f9e8
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

console.debug("Received a repsonse that doesn't have a /jsonapi/version property")

// Don't even bother if the data propery doesn't exist
return typeof response.data !== 'undefined';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be doc.data?

@jagthedrummer
Copy link

Anything we can do to get this merged? I just ran into this and am trying to work around it like so, but with no luck.

    after(response){
      response.jsonapi = { "version": "1.0" };
      serializeAndPush(this, response);
    }

I'm still getting the console message:

serializeAndPush may only be used with a JSON API document. Ignoring response. Document must have a mandatory JSON API object. See https://jsonapi.org/format/#document-jsonapi-object.

@dbollinger
Copy link

dbollinger commented Apr 6, 2020

@jagthedrummer I was running into friction here as well (literally yesterday) -- in the short term, so that I could start using this add-on, I just created my own (very brittle) method:

export default function serializeAndPushRecord(payload) {
  let serializer = this.store.serializerFor(payload.data.type);
  serializer.pushPayload(this.store, payload);
}

YMMV, and I only need this for single members (not collections) right now, but if you're looking for a quick answer while this addon goes through some modernization and maintenance, something like this could be an option.

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

Successfully merging this pull request may close these issues.

None yet

4 participants