Default body to querystring, null encoding#892

flovilmart · 2016-03-07T16:07:27Z

fixes #727

sets the default encoding to null to prevent parsing to utf-8 upon reception as per: no decode delegate for this image format `' @ error/constitute.c/ReadImage/501. flovilmart/parse-image#7

encoding - Encoding to be used on setEncoding of response data. If null, the body is returned as a Buffer. Anything else (including the default value of undefined) will be passed as the encoding parameter to toString() (meaning this is effectively utf8 by default). (Note: if you expect binary data, you should set encoding: null.)

gfosco · 2016-03-07T16:56:06Z

spec/HTTPRequest.spec.js

-  it("should encode a JSON body", (done) => {
+  it("should encode a JSON body by default", (done) => {
+
+    let options = {


we need 'use strict' at the top if you want to use let.

facebook-github-bot · 2016-03-07T18:01:23Z

@flovilmart updated the pull request.

codecov-io · 2016-03-07T18:02:46Z

Current coverage is `91.03%`

Merging #892 into master will increase coverage by +0.01% as of ebb0d86

@@            master    #892   diff @@
======================================
  Files           72      73     +1
  Stmts         4165    4171     +6
  Branches       862     863     +1
  Methods          0       0       
======================================
+ Hit           3791    3797     +6
  Partial         10      10       
  Missed         364     364

Review entire Coverage Diff as of ebb0d86

Powered by Codecov. Updated on successful CI builds.

drew-gross · 2016-03-07T19:54:27Z

Looks good to me but since this is a fairly big API change I would prefer having a 2nd reviewer.

facebook-github-bot · 2016-03-08T04:23:21Z

@flovilmart updated the pull request.

nlutsenko · 2016-03-08T04:24:50Z

spec/HTTPRequest.spec.js

    })
  });
-  it("should encode a JSON body", (done) => {
+  it("should encode a JSON body by default", (done) => {


Nit: Space before.

nlutsenko · 2016-03-08T04:32:23Z

Put few thoughts here and there, nothing very bad, but few pieces that feel weird...
Also, if we are using ES6 - can we use it on every line you changed? This is nitpicking... I know...

facebook-github-bot · 2016-03-08T04:54:12Z

@flovilmart updated the pull request.

facebook-github-bot · 2016-03-08T05:17:14Z

@flovilmart updated the pull request.

facebook-github-bot · 2016-03-08T13:06:32Z

@flovilmart updated the pull request.

facebook-github-bot · 2016-03-08T13:12:13Z

@flovilmart updated the pull request.

facebook-github-bot · 2016-03-08T13:17:19Z

@flovilmart updated the pull request.

flovilmart · 2016-03-11T04:02:45Z

@nlutsenko what's your thoughts on that? The default encoding will be QS, to match with original implementation of httpRequest

nlutsenko · 2016-03-17T02:37:34Z

Looks good, sorry took a while to review.
Matching original encoding of httpRequest sounds good as well.

nlutsenko · 2016-03-17T02:38:35Z

src/cloud-code/HTTPResponse.js

+    this.cookies = response.headers["set-cookie"];
+  }
+
+  get text() {


Love the lazy-loading piece here.

…ultContentType Default body to querystring, null encoding

facebook-github-bot added the GH Review: review-needed label Mar 7, 2016

gfosco reviewed Mar 7, 2016
View reviewed changes

flovilmart force-pushed the flovilmart.httpRequestDefaultContentType branch from 8dec808 to 647532f Compare March 7, 2016 17:56

flovilmart changed the title ~~Encodes the body by default to application/json if is object~~ Default body to JSON, null encoding Mar 8, 2016

flovilmart assigned nlutsenko Mar 8, 2016

nlutsenko reviewed Mar 8, 2016
View reviewed changes

nlutsenko assigned flovilmart and unassigned nlutsenko Mar 8, 2016

nlutsenko added GH Review: needs-revision and removed GH Review: review-needed labels Mar 8, 2016

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Mar 8, 2016

flovilmart force-pushed the flovilmart.httpRequestDefaultContentType branch from 2c30e38 to 66fefa5 Compare March 8, 2016 05:16

flovilmart added 4 commits March 8, 2016 07:54

Encodes the body by default to application/json if is object

8300dd8

Sets encoding to null to preserve raw buffers

f40efe3

Adds HTTPResponse object and lazy loading text and JSON

982fc72

ES6-ify

6acb5ce

flovilmart force-pushed the flovilmart.httpRequestDefaultContentType branch from 66fefa5 to 6acb5ce Compare March 8, 2016 13:04

default encoding is now querystring instead of JSON

bd7d951

flovilmart force-pushed the flovilmart.httpRequestDefaultContentType branch from 2c183f8 to bd7d951 Compare March 8, 2016 13:12

flovilmart changed the title ~~Default body to JSON, null encoding~~ Default body to querystring, null encoding Mar 8, 2016

flovilmart assigned nlutsenko and unassigned flovilmart Mar 8, 2016

nlutsenko added GH Review: accepted and removed GH Review: review-needed labels Mar 17, 2016

nlutsenko assigned flovilmart and unassigned nlutsenko Mar 17, 2016

nlutsenko reviewed Mar 17, 2016
View reviewed changes

flovilmart added a commit that referenced this pull request Mar 17, 2016

Merge pull request #892 from ParsePlatform/flovilmart.httpRequestDefa…

0f7335b

…ultContentType Default body to querystring, null encoding

flovilmart merged commit 0f7335b into master Mar 17, 2016

flovilmart deleted the flovilmart.httpRequestDefaultContentType branch March 17, 2016 02:43

cleever mentioned this pull request Jun 24, 2016

Parse.cloud.httpRequest buffer #2033

Closed

Uh oh!

Conversation

flovilmart commented Mar 7, 2016

Uh oh!

gfosco Mar 7, 2016

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Mar 7, 2016

Uh oh!

codecov-io commented Mar 7, 2016

Current coverage is 91.03%

Uh oh!

drew-gross commented Mar 7, 2016

Uh oh!

facebook-github-bot commented Mar 8, 2016

Uh oh!

nlutsenko Mar 8, 2016

Choose a reason for hiding this comment

Uh oh!

nlutsenko commented Mar 8, 2016

Uh oh!

facebook-github-bot commented Mar 8, 2016

Uh oh!

facebook-github-bot commented Mar 8, 2016

Uh oh!

facebook-github-bot commented Mar 8, 2016

Uh oh!

facebook-github-bot commented Mar 8, 2016

Uh oh!

facebook-github-bot commented Mar 8, 2016

Uh oh!

flovilmart commented Mar 11, 2016

Uh oh!

nlutsenko commented Mar 17, 2016

Uh oh!

nlutsenko Mar 17, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Current coverage is `91.03%`