Skip to content

handle headers as both strings and arrays#84

Closed
kellyselden wants to merge 2 commits intoember-fastboot:masterfrom
kellyselden:headers-fix
Closed

handle headers as both strings and arrays#84
kellyselden wants to merge 2 commits intoember-fastboot:masterfrom
kellyselden:headers-fix

Conversation

@kellyselden
Copy link
Copy Markdown
Member

When I don't supply the express response in the visit options, everything works fine, but when I do supply it:

app.visit(url, {
  response: res
})

The headers parsing fails because sometimes the header values are already arrays and don't have the split function. A sample while debugging:

// values masked...
debug> exec headers
{ 'set-cookie': 
   [ 'ident=14xxxxxxxxx24; Max-Age=1800; Doma... (length: 160)',
     'session=0xxxxxxxxD; Domain=test.com; Path=/;... (length: 89)' ],
  'x-session-id': '5xxxxxxxxx4',
  'x-registration-id': '8xxxxxxx4',
  'x-appserver-id': 'test',
  token: 'eyJhbGciOiJIUzxxxxxxxxxxxxxxxxxRDVGQkR... (length: 1636)' }

I don't yet know if this is an issue only I would have for some reason, or it will start turning up in other apps.

@kellyselden
Copy link
Copy Markdown
Member Author

I should add that I'm using the standalone fastboot package, without any middleware.

Copy link
Copy Markdown

@arjansingh arjansingh left a comment

Choose a reason for hiding this comment

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

Other than the type checking for a string... LGTM.

Comment thread src/fastboot-headers.js Outdated
let value = headers[header];

// convert to array if not already
if (value.split) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd prefer doing an instanceof or typeof check here instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

changed.

@kellyselden
Copy link
Copy Markdown
Member Author

I finally found the evidence needed to get this merged. Express turns the set-cookie response header into an array if you set more than one cookie:

https://github.com/expressjs/express/blob/bd47aeb88d28c6cff6e2d8e5378ec73c262e039f/lib/response.js#L805
https://github.com/expressjs/express/blob/bd47aeb88d28c6cff6e2d8e5378ec73c262e039f/lib/response.js#L682

@kellyselden
Copy link
Copy Markdown
Member Author

cc @danmcclain and @tomdale, I remember on the call you two expressed concern over this change.

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Jan 13, 2017

closed by #113

@rwjblue rwjblue closed this Jan 13, 2017
@kellyselden kellyselden deleted the headers-fix branch January 13, 2017 23:50
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