options.type accepts array #268

shawninder · 2017-09-25T15:56:15Z

I just forked an existing PR and added what was missing for it to be merged. See the original conversation.

Summary:
heme:

Seems like your type property in options correctly passes an array on to type-is. Wasn't immediately obvious I could do that based on the README.

dougwilson:

Awesome, thanks! Can you go ahead and also add it to all the other parsers as well? Currently I believe all their type sections are identical, so let's keep them identical.

So that's what I did. Added the change to all the other parsers as well :)

…ings)

dougwilson · 2017-09-25T17:08:36Z

Nice @shawninder ! I haven't dug into the changes yet, but just wanted to note that the new tests added here are failing in case you didn't realize.

dougwilson · 2017-09-25T17:09:02Z

README.md

@@ -132,7 +133,7 @@ to `'100kb'`.
 ##### type

 The `type` option is used to determine what media type the middleware will
-parse. This option can be a function or a string. If a string, `type` option
+parse. This option can be a string, array of strings, or a function. If it is a string or array of strings, `type` option


wrap the line here to ~80 chars

dougwilson · 2017-09-25T17:09:07Z

README.md

@@ -180,7 +181,7 @@ to `'100kb'`.
 ##### type

 The `type` option is used to determine what media type the middleware will
-parse. This option can be a function or a string. If a string, `type` option
+parse. This option can be a string, array of strings, or a function. If it is a string or array of strings, `type` option


wrap the line here to ~80 chars

dougwilson · 2017-09-25T17:09:11Z

README.md

@@ -243,7 +244,7 @@ than this value, a 413 will be returned to the client. Defaults to `1000`.
 ##### type

 The `type` option is used to determine what media type the middleware will
-parse. This option can be a function or a string. If a string, `type` option
+parse. This option can be a string, array of strings, or a function. If it is a string or array of strings, `type` option


wrap the line here to ~80 chars

shawninder · 2017-09-25T18:24:40Z

For me, the only test that fails is a test that was already failing before I made any changes. In other words, the tests added in this PR do pass, at least for me...

Here is the full output:

$ npm test

> body-parser@1.15.0 test /SOME_PATH/body-parser
> mocha --require test/support/env --reporter spec --check-leaks --bail test/



  bodyParser()
    ✓ should default to {}
    ✓ should parse JSON
    ✓ should parse x-www-form-urlencoded
    ✓ should handle duplicated middleware
    http methods
      ✓ should support ACL requests
      ✓ should support BIND requests
      ✓ should support CHECKOUT requests
      ✓ should support COPY requests
      ✓ should support DELETE requests
      ✓ should support GET requests
      ✓ should support HEAD requests
      ✓ should support LINK requests
      ✓ should support LOCK requests
      ✓ should support M-SEARCH requests
      ✓ should support MERGE requests
      ✓ should support MKACTIVITY requests
      ✓ should support MKCALENDAR requests
      ✓ should support MKCOL requests
      ✓ should support MOVE requests
      ✓ should support NOTIFY requests
      ✓ should support OPTIONS requests
      ✓ should support PATCH requests
      ✓ should support POST requests
      ✓ should support PROPFIND requests
      ✓ should support PROPPATCH requests
      ✓ should support PURGE requests
      ✓ should support PUT requests
      ✓ should support REBIND requests
      ✓ should support REPORT requests
      ✓ should support SEARCH requests
      ✓ should support SUBSCRIBE requests
      ✓ should support TRACE requests
      ✓ should support UNBIND requests
      ✓ should support UNLINK requests
      ✓ should support UNLOCK requests
      ✓ should support UNSUBSCRIBE requests
    with type option
      ✓ should parse JSON
      ✓ should parse x-www-form-urlencoded
    with verify option
      ✓ should apply to json
      ✓ should apply to urlencoded

  bodyParser.json()
    ✓ should parse JSON
    1) should fail gracefully


  41 passing (136ms)
  1 failing

  1) bodyParser.json() should fail gracefully:
     Error: expected 'Unexpected end of input' response body, got 'Unexpected end of JSON input'
      at error (node_modules/supertest/lib/test.js:265:13)
      at Test._assertBody (node_modules/supertest/lib/test.js:191:16)
      at Test._assertFunction (node_modules/supertest/lib/test.js:247:11)
      at Test.assert (node_modules/supertest/lib/test.js:148:18)
      at Server.assert (node_modules/supertest/lib/test.js:127:12)
      at emitCloseNT (net.js:1553:8)
      at _combinedTickCallback (internal/process/next_tick.js:71:11)
      at process._tickCallback (internal/process/next_tick.js:98:9)



npm ERR! Test failed.  See above for more details.

…tps://atom.io/packages/line-length-break) atom package

dougwilson · 2017-09-25T18:29:25Z

That's strange, because master is passing in Travis CI and this PR was not. I can't merge until Travis CI passes on the PR, so whatever you think is the necessary action, please take it.

dougwilson · 2017-09-25T18:32:09Z

That message looks like you are using a really old version of this module on a Node.js version that veesion doesn't support. Either use an older Node.js version or rebase this on to the current master (rebase; do not merge or I can't merge the PR).

…pe` option

shawninder · 2017-09-25T18:40:08Z

I did find one of my tests wasn't passing. Didn't notice at first because the failing test was preventing the tests from going further... I can now confirm that the tests you and I added now all pass. Only the tests that previously failed still fail...

As for using a really old version of the module, I just forked your PR @dougwilson. Perhaps it was already very old or something? Anyways, my version of node shouldn't have any impact on the CI server, which shows all green now :)

dougwilson · 2017-09-25T18:41:55Z

I just forked your PR @dougwilson. Perhaps it was already very old or something?

What PR? I didn't have ant PR that I'm aware of. Are you sure thr PR was mine and not someone elses? What is the date of that original PR?

shawninder · 2017-09-25T18:42:05Z

My previous comment should have read "I just forked your PR @heme"

dougwilson · 2017-09-25T18:43:43Z

Oh, I see the link to the PR at the top. The PR qas made 1 year ago, so you should not use a version of Node.js that didn't exist 1 year ago to run your test locally.

shawninder · 2017-09-25T18:43:46Z

Only the differences I introduced show up in this PR, so it looks to me like I forked from the latest commit on master (not old at all). Am I missing something here?

shawninder · 2017-09-25T18:44:28Z

Ah, I understand. The code is still latest, but was last developed against the a previous version of Node.js, and running the tests against my version of Node.js (v6.9.1) breaks the tests. Makes sense. Still, the CI server is seeing all tests pass. Is that sufficient to merge?

dougwilson · 2017-09-25T18:46:07Z

Should be, but I haven't actually taken a look over the PR, instead spent my break time arguing about the test failures :) I have to get back to work, will check after work.

shawninder · 2017-09-25T18:49:31Z

Sounds good man, thanks!

dougwilson · 2017-09-26T19:24:52Z

Sorry I didn't get to this yesterday. I'm working to merge now, but fixing the wrapping issues and the old testing pattern that is being used that was changed.

heme and others added 2 commits March 16, 2016 15:39

options.type can take an array of strings for content-type

e7e0dff

Document and test existing feature (type option accepts arrays of str…

e79d40f

…ings)

shawninder mentioned this pull request Sep 25, 2017

options.type accepts array #163

Closed

dougwilson added docs tests pr labels Sep 25, 2017

dougwilson reviewed Sep 25, 2017

View reviewed changes

Wrap lines in README.md to ~80 chars using the [line-length-break](ht…

870226e

…tps://atom.io/packages/line-length-break) atom package

Fix test for text parser when handling an array of strings in the `ty…

e8ccf51

…pe` option

dougwilson closed this in 090a92b Sep 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

options.type accepts array #268

options.type accepts array #268

shawninder commented Sep 25, 2017

dougwilson commented Sep 25, 2017

dougwilson Sep 25, 2017

dougwilson Sep 25, 2017

dougwilson Sep 25, 2017

shawninder commented Sep 25, 2017 •

edited

Loading

dougwilson commented Sep 25, 2017

dougwilson commented Sep 25, 2017

shawninder commented Sep 25, 2017

dougwilson commented Sep 25, 2017

shawninder commented Sep 25, 2017

dougwilson commented Sep 25, 2017

shawninder commented Sep 25, 2017

shawninder commented Sep 25, 2017 •

edited

Loading

dougwilson commented Sep 25, 2017

shawninder commented Sep 25, 2017

dougwilson commented Sep 26, 2017

options.type accepts array #268

options.type accepts array #268

Conversation

shawninder commented Sep 25, 2017

dougwilson commented Sep 25, 2017

dougwilson Sep 25, 2017

Choose a reason for hiding this comment

dougwilson Sep 25, 2017

Choose a reason for hiding this comment

dougwilson Sep 25, 2017

Choose a reason for hiding this comment

shawninder commented Sep 25, 2017 • edited Loading

dougwilson commented Sep 25, 2017

dougwilson commented Sep 25, 2017

shawninder commented Sep 25, 2017

dougwilson commented Sep 25, 2017

shawninder commented Sep 25, 2017

dougwilson commented Sep 25, 2017

shawninder commented Sep 25, 2017

shawninder commented Sep 25, 2017 • edited Loading

dougwilson commented Sep 25, 2017

shawninder commented Sep 25, 2017

dougwilson commented Sep 26, 2017

shawninder commented Sep 25, 2017 •

edited

Loading

shawninder commented Sep 25, 2017 •

edited

Loading