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

Bring some sanity to request parsing and error handling #2335

Merged
merged 2 commits into from
May 29, 2019
Merged

Bring some sanity to request parsing and error handling #2335

merged 2 commits into from
May 29, 2019

Conversation

dead-claudia
Copy link
Member

@dead-claudia dead-claudia commented Dec 10, 2018 •
edited
Loading

Description

  • The browser can do JSON parsing itself. Let's defer to that where possible. (A few IE hacks are required here, though.)
  • Don't propagate any error that occurs before deserialize/extract.
  • Allow sending raw array buffers/blobs/etc. to deserialize.
  • Align behavior more closely with the XHR spec.
  • Send the more useful parsed response to deserialize, not the less useful string response.

Motivation and Context

Fixes #2333
Edit: Fixes #2313

Here's why I included more breaking changes:

  1. I wanted to not hard-code all the possible values for responseType.
  2. I noticed that XHR has had this convenient responseType: "json" feature for a while and that it's fully integrated with response, and thought that could simplify our end a little.
  3. I didn't want to slap on yet another band-aid, especially when the docs were already starting to show signs of it being covered in them.

How Has This Been Tested?

Ran all the tests and updated them as appropriate. I updated a mock, but the tests for them didn't really need updated.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@dead-claudia dead-claudia added Type: Bug For bugs and any other unexpected breakage Type: Breaking Change For any feature request or suggestion that could reasonably break existing code labels Dec 10, 2018
@dead-claudia dead-claudia added this to the 2.0.0 milestone Dec 10, 2018
@dead-claudia dead-claudia requested a review from a team December 10, 2018 09:33
@dead-claudia dead-claudia force-pushed the request-response-type-fix branch from 37bd8ac to c2b1eb7 Compare December 10, 2018 09:34
@dead-claudia
Copy link
Member Author

@MithrilJS/collaborators Could I get a review here?

@dead-claudia dead-claudia requested review from barneycarroll and StephanHoyer and removed request for a team February 1, 2019 21:28
@dead-claudia dead-claudia force-pushed the request-response-type-fix branch from c2b1eb7 to 1ce2376 Compare February 7, 2019 09:38
@dead-claudia dead-claudia force-pushed the request-response-type-fix branch 3 times, most recently from 4999b46 to 42002f4 Compare March 2, 2019 07:40
StephanHoyer
StephanHoyer requested changes Mar 14, 2019
View reviewed changes
docs/request.md Outdated Show resolved Hide resolved
@dead-claudia
Copy link
Member Author

@StephanHoyer Anything else stick out to you?

@dead-claudia dead-claudia removed the request for review from barneycarroll May 12, 2019 09:42
dead-claudia and others added 2 commits May 29, 2019 09:35
@dead-claudia
Bring some sanity to request parsing and error handling
0527277 
- The browser can do JSON parsing itself. Let's defer to that where
  possible. (A few IE hacks are required here, though.)
- Don't propagate any error that occurs before `deserialize`/`extract`.
- Allow sending raw array buffers/blobs/etc. to `deserialize`.
- Align behavior more closely with the XHR spec.
- Send the more useful parsed response to `deserialize`, not the less
  useful string response.
@StephanHoyer @dead-claudia
Update docs/request.md
b814567 
Co-Authored-By: isiahmeadows <contact@isiahmeadows.com>
@dead-claudia dead-claudia force-pushed the request-response-type-fix branch from cf9f4ab to b814567 Compare May 29, 2019 13:35
@dead-claudia
Copy link
Member Author

Merging as per private discussion with @StephanHoyer.

@dead-claudia dead-claudia merged commit 794e8e9 into MithrilJS:next May 29, 2019
@dead-claudia dead-claudia deleted the request-response-type-fix branch May 29, 2019 13:41
@dead-claudia dead-claudia restored the request-response-type-fix branch May 29, 2019 13:42
@dead-claudia dead-claudia deleted the request-response-type-fix branch May 29, 2019 14:16
@snyk-bot snyk-bot mentioned this pull request Feb 2, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuzetsu fuzetsu fuzetsu left review comments

@StephanHoyer StephanHoyer StephanHoyer requested changes

Assignees
No one assigned
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Bug For bugs and any other unexpected breakage
Projects
Triage/bugs
Status: Closed
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

Request fails when using non-text responseType m.request error handling is not very useful
3 participants
@dead-claudia @StephanHoyer @fuzetsu