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

change response type parsing logic #25

Merged
merged 7 commits into from
Dec 4, 2024
Merged

Conversation

yougyung
Copy link
Member

@yougyung yougyung commented Nov 19, 2024

📌 작업 내용

구현 내용 및 작업 했던 내역

#20

  • reponse type parsing 로직의 변경

문제

  • responseType는 undefined를 허용하며, default로 json을 갖음 -> 하지만 응답 본문이 없는 경우에도 responseType가 json이므로 response.json()으로 파싱되는 문제가 존재

변경된 로직

  1. responseType을 요청한 경우 -> responseType으로 parsing
  2. responseType을 요청하지 않은 경우 -> 원본 데이터를 반환하되, content-type이 application/json인 경우에 한해서 json으로 parsing

원본데이터를 반환하는 경우

  • parsing에 실패하거나 Content-Type이 명시되지 않고 json형식이 아닌 경우, 원본 데이터를 그대로 반환

🤔 고민 했던 부분

  1. 우선순위

    • responseType이 있는 경우
    • content type = application/json 인 경우
      → responseType을 우선 순위로 함
  2. 에러가 발생한 경우 parsing type

    • 파싱이 실패하면 Axios는 해당 데이터의 body를 그대로 반환하며, error를 throw하지 않음
  3. parsing 될 값이 유추되는 content-type에 대해서 자동 parsing 기능 구현 -> 진행하지 않음

    • axios에서는 application/json이 아닌 타입에 대해서 원본 데이터를 그대로 반환하고 있음 -> 사용자가 axios와 동일하게 응답이 될 것이라는 기대가 존재
    • content-type이 json의 형태라고 하더라도 데이터가 어떤 맥락에서 사용되는지는 일치하지 않을 수 있으므로, .json()으로 무조건 파싱하는 것은 데이터의 컨텍스트 잃을 수 있다고 판단하여 원본 데이터의 반환을 통해 사용자가 유연하게 설계하도록 구현

위 에러 해결 과정도, 조금 더 구체적이게 졸부 노션의 문서자료-fetch-기타자료에 추가했습니다! https://soft-anorak-0ca.notion.site/responseType-json-parsing-514e09fbfde84edcb6f0be085b7af88a?pvs=4

@yougyung yougyung self-assigned this Nov 19, 2024
Copy link
Contributor

@seonghunYang seonghunYang left a comment

Choose a reason for hiding this comment

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

  • 중요한 작업이네요. 수고하셨습니다!
  • 다만 현재 responseType의 유무로 설정하고 있는데, presetOption이 기본적으로 json으로 설정되어 있어서 사용자가 따로 설정하지 않았을 때 유경님이 의도한 대로 동작하지 않을 수 있겠다는 생각이 드네요.

@yougyung
Copy link
Member Author

  • 중요한 작업이네요. 수고하셨습니다!
  • 다만 현재 responseType의 유무로 설정하고 있는데, presetOption이 기본적으로 json으로 설정되어 있어서 사용자가 따로 설정하지 않았을 때 유경님이 의도한 대로 동작하지 않을 수 있겠다는 생각이 드네요.

논리에 오류가있었네요! 집어주셔서 감사합니다.

responseType의 default를 json로 설정한 것은 axios의 reponseType의 영향도 있지만, 결과적으로 응답이 json으로 parsing될 확률이 제일 높을 것이라고 예상되었기 때문입니다.
content-type이 application/json인 경우에 json parsing이 실행하여 위 목적을 만족시키니, responseType의 명시적으로 선언되어져있는 default 값을 제거하여 개발자의 유연한 설계를 보장하는 방향으로 수정하는 것은 어떨까요? 팀원분들의 의견 궁금합니다!

@yougyung
Copy link
Member Author

responseType이 있으면 content-type과 관계없이 responseType으로 파싱하고,
responseType이 없으면, 원본 응답을 반환하나 content-type이 Application/json인 경우에만 예외를 둬 json으로 파싱한다

요 결론으로 이해해주시면 됩니다!

Copy link
Member

@gahyuun gahyuun left a comment

Choose a reason for hiding this comment

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

제가 처음에 task 요청 드렸을 때는 content-type에 맞게 파싱하는 기능이었는데, 조사하신 거 보니까 그 방식이 훨씬 괜찮네요! 많은 고민이 느껴져요 고생하셨습니다 ㅎㅎ
또한 content-type이 application/json일 경우 responseType이 없어도 파싱을 해주기 때문에 responseType의 default 값은 제거해도 좋을 것 같아요!
만약 이 부분의 수정이 진행된다면 README도 수정과 배포까지..!! 부탁드릴게요 😊

src/index.ts Outdated
Comment on lines 140 to 145
if (!resolvedResponseType)
return buildFetchAXResponse<T>(response, response.body as T);

try {
const data = await parseResponseData<T>(response, resolvedResponseType);
return buildFetchAXResponse(response, data);
Copy link
Member

Choose a reason for hiding this comment

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

제가 코드를 간단하게 보았을 땐, parseResponseData 함수에서 type이 undefined일 경우 default문으로 들어가 따로 파싱을 진행하지 않는 흐름일 것 같아요!
위 흐름이 맞다면 아래 코드가 필요 없지 않을까요??

if (!resolvedResponseType)
    return buildFetchAXResponse<T>(response, response.body as T);

Copy link
Member Author

Choose a reason for hiding this comment

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

1.resolveResponseType함수에서 undefined를 반환하는 케이스는
2. 원본 객체를 반환한다
개발 당시 의도는 위를 명시적으로 표현하기 위함이었어요.

resolveResponseType함수를 통해 해당 내용을 유추할 수 있고 parseResponseData의 처리 흐름과 중복되다는 점을 보아 제거해도 좋겠네요! 의견 감사합니다.

@seonghunYang
Copy link
Contributor

responseType이 있으면 content-type과 관계없이 responseType으로 파싱하고, responseType이 없으면, 원본 응답을 반환하나 content-type이 Application/json인 경우에만 예외를 둬 json으로 파싱한다

요 결론으로 이해해주시면 됩니다!

그렇다면 responseType의 기본 설정을 제거하고 해당 방식으로 처리하자는 말씀 맞을까요?

@yougyung
Copy link
Member Author

responseType이 있으면 content-type과 관계없이 responseType으로 파싱하고, responseType이 없으면, 원본 응답을 반환하나 content-type이 Application/json인 경우에만 예외를 둬 json으로 파싱한다

요 결론으로 이해해주시면 됩니다!

그렇다면 responseType의 기본 설정을 제거하고 해당 방식으로 처리하자는 말씀 맞을까요?

네 맞습니다!

@yougyung yougyung merged commit f3256a3 into main Dec 4, 2024
@yougyung yougyung deleted the fix/responseType-parsing branch December 4, 2024 08:58
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.

3 participants