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.

Sign up for GitHub

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

Jump to bottom

[RK] Transform request & response fix, add documentation #53

Merged
merged 4 commits into from
Nov 1, 2021
Merged

[RK] Transform request & response fix, add documentation #53

merged 4 commits into from
Nov 1, 2021

Conversation

regan-karlewicz
Copy link
Contributor

@regan-karlewicz regan-karlewicz commented Oct 13, 2021 •
edited
Loading

Issue Link

Issue 6

Description

  • Add default axios transform to response, eliminates need to JSON parse in response transform
  • Add default axios transform to request. Current transform request is broken because it is not using the default axios transform.
  • Add documentation for this in README

PR Requirements

Before requesting review this criteria must be met:

  • Overall test coverage >= current master?
  • Documentation included -- README.md updated, docs, etc. (if any behavioral changes)?
  • Backwards compatibility (if breaking change)?

@regan-karlewicz regan-karlewicz changed the title [RK] Transform request & response fix, [RK] Transform request & response fix, add documentation Oct 13, 2021
@regan-karlewicz regan-karlewicz mentioned this pull request Oct 13, 2021
Closed
regan-karlewicz
regan-karlewicz commented Oct 13, 2021
View reviewed changes
src/Route.ts
Comment on lines +106 to +107
private transformRequestFn = axios.defaults.transformRequest as AxiosTransformer[];
private transformResponseFn = axios.defaults.transformResponse as AxiosTransformer[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we allow the ability to not use the default transformer? This is what would cause the breaking change for applications already using transformResponse.

@regan-karlewicz regan-karlewicz requested review from stephenbaldwin and MatthewCMoya and removed request for stephenbaldwin October 13, 2021 19:11
@regan-karlewicz regan-karlewicz merged commit f83881d into master Nov 1, 2021
@regan-karlewicz regan-karlewicz deleted the transforms/document-and-fix branch November 1, 2021 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MatthewCMoya MatthewCMoya MatthewCMoya approved these changes

@noahgribbin noahgribbin noahgribbin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@regan-karlewicz @MatthewCMoya @noahgribbin