Skip to content

Commit

Permalink
[BUGFIX fetch] Don't set json:api content-type for DELETE requests
Browse files Browse the repository at this point in the history
  • Loading branch information
@dcyriller @runspired
dcyriller authored and runspired committed Mar 25, 2019
1 parent a61651d commit f26d07b
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions addon/adapters/json-api.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,11 @@ const JSONAPIAdapter = RESTAdapter.extend({
@param {Object} options
@return {Object}
*/
ajaxOptions(url, type, options) {
options = options || {};
options.contentType = 'application/vnd.api+json';
ajaxOptions(url, type, options = {}) {
if (['POST', 'PUT', 'PATCH'].includes(type)) {
options.contentType = 'application/vnd.api+json';
}

let hash = this._super(url, type, options);
hash.headers['Accept'] = 'application/vnd.api+json';
return hash;
Expand Down

6 comments on commit f26d07b

@victornamuso
Copy link

Choose a reason for hiding this comment

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

@dcyriller @runspired This change actually seems to break what it seemed to attempt to fix.

Needed to override ajaxOptions in my adapter to get deletes working again:

 ajaxOptions(url, type, options = {}) {
    if (['POST', 'PUT', 'PATCH','DELETE'].includes(type)) {
      options.contentType = 'application/vnd.api+json';
    }

    let hash = this._super(url, type, options);
    hash.headers['Accept'] = 'application/vnd.api+json';
    return hash;
  },

Either it should go back to the way it was or, 'DELETE' needs to be added to the array like in my code sample.

@runspired
Copy link
Contributor

Choose a reason for hiding this comment

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

@victornamuso why would your delete requests expect this header?

@runspired
Copy link
Contributor

Choose a reason for hiding this comment

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

That said it does seem that jsonapi wants us to set this header regardless: https://jsonapi.org/format/#crud-deleting

@victornamuso
Copy link

Choose a reason for hiding this comment

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

@runspired Not sure. Trying to dig around our api to see why it's throwing an error if the contentType isn't set.

@dcyriller
Copy link
Contributor Author

@dcyriller dcyriller commented on f26d07b Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That said it does seem that jsonapi wants us to set this header regardless: https://jsonapi.org/format/#crud-deleting

The spec is about the response (Accept) not the request (Content-Type) for DELETEs @runspired ;) EDIT: this is wrong

I've opened a PR to revert this change, it was breaking: #5999

Sorry about that @victornamuso

@drauschenbach
Copy link

Choose a reason for hiding this comment

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

@victornamuso to answer your question why the header is needed, I have a router in a content-negotiating backend that routes all JSONAPI related requests to a CRUD layer, then some other application/json related requests go to some other CRUD layer that happens to not support deletes, then all other requests to a static web serving layer associated with a webroot directory on the filesystem. In my case, removal of this header would completely break my app. It seemed like a heavy-handed change.

Please sign in to comment.