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

General improvements #13

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

General improvements #13

wants to merge 14 commits into from

Conversation

rdiazv
Copy link
Contributor

@rdiazv rdiazv commented Mar 28, 2018

This works in conjunction with masylum/mobx-rest/pull/39.

Changes

  • Adds methods for patch and head requests.
  • Adds method errorUnwrap that keeps the previous behavior of returning error.errors || {} when the request fails, but allows overriding.
  • Allows to pass options to qs.stringify. In my particular case I'm doing this:
apiClient(adapter, {
  defaults: { // Previously `commonOptions`
    qs: {
      arrayFormat: 'brackets'
    }
  }
})

Breaking changes

  • Renames apiPath to urlRoot.
  • Renames commonOptions to defaults.
  • Removes fetch-ponyfill. If the host app already has a polyfill this will just be adding unused bytes. I think the developer should include a fetch polyfill in his app, if needed.
  • Rejections now returns { requestResponse, error }, requestResponse being the original request response object and error the result of errorUnwrap. This allows to get response headers, status, etc.

Fixes

Notes

We aren't using the jquery-adapter, so these changes will have to be applied/tested there also.

@rdiazv rdiazv changed the title General Improvements General improvements Mar 28, 2018
@rdiazv rdiazv mentioned this pull request Mar 28, 2018
@masylum
Copy link
Owner

masylum commented Mar 29, 2018

Will check this. Looking pretty solid.

Object.assign(baseHeaders, headers)
)

return {
...otherOptions,
headers: headersObject,
body: data ? JSON.stringify(data) : null
body: data ? JSON.stringify(data) : undefined
Copy link
Owner

Choose a reason for hiding this comment

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

just to understand, why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this JakeChampion/fetch#402. Particularly, Edge was failing with the error Failed to execute 'fetch()' on 'Window': HEAD or GET Requests cannot have a body.

Related: agraboso/redux-api-middleware#138

Copy link
Owner

@masylum masylum left a comment

Choose a reason for hiding this comment

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

Looking good. I can't merge this until releasing the new version of mobx-rest though. The breaking changes must be applied there first.

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.

2 participants