add serializeObject export to serialize multiple cookies at a time

[ryhinchey]

Proposal for issue #30

Alternative approach from PR #47

var cookie = require('cookie');

var cookies = cookie.serializeObject({foo: 'bar', bar: 'foo'}, {httpOnly: true});
// ['foo=bar; HttpOnly', 'bar=foo; HttpOnly']

[dougwilson]

Thank you!

Alternative approach from PR #47

Can you elaborate on this point? What is changed between this PR and that and why?

[ryhinchey]

Thank you!

Alternative approach from PR #47

Can you elaborate on this point? What is changed between this PR and that and why?

@dougwilson PR #47 modifies serialize to work with an object of key/value pairs:

// current functionality
cookie.serialize('foo', 'bar', {});

// PR addition
cookie.serialize({foo: 'bar'}, {});

It also adds a function serializeNameValue which would be used on both approaches above that does most of what serialize currently does.

Looking at the PR, the parameters name, val, and options are confusing when an object is being passed as name and options as val.

A comment mentioned serialize being "overloaded" which I think is true and would lead to potential confusion down the road.

This PR keeps serialize as is and adds a new method specifically for passing multiple cookie name/value pairs as an object.

cookie.serializeObject({foo: 'bar', bar: 'foo'}, {httpOnly: true});
// ['foo=bar; HttpOnly', 'bar=foo; HttpOnly']

That said, supporting multiple key/value pairs is something that can easily be handled outside of this library as well! I think it's perfectly reasonable for multiple key/value pairs to be considered beyond the scope of this library and if you'd prefer not to add any of these implementations, I'm fine with that 😄

[jamesarosen]

Because serialize is the inverse of parse (under the degenerate case of one cookie), I would have expected cookie.parse(cookie.serializeObject(obj)) to work, but your serializeObject returns a String[] rather than a single cookie header value.

This is certainly a matter of preference. I can't think of a use-case where I'd want the individual serialized strings, but I don't doubt that such a case exists.

[ryhinchey]

@jamesarosen my thinking was that folks might want to have multiple Set-Cookie headers and returning a single string to pass to Set-Cookie would make that use case a bit more tedious. I'm up for changing serializeObject to return a string if it's preferred though.

[jamesarosen]

my thinking was that folks might want to have multiple Set-Cookie headers

I keep thinking that Set-Cookie has to be separated by options, but in fact each key/value pair must be in a unique Set-Cookie header. That is,

Set-Cookie: foo=bar; baz=quux; HttpOnly; Secure

is not valid (though I really wish it were).

[wesleytodd]

LGTM!

EDIT: I feel like the GitHub UX around that is weird, I didn't realize that "comment" was default and accidentally clicked that, resulting in two notifications and a separate approval and comment action....

[dougwilson]

I think I am with @ryhinchey in that this is probably outside the scope of this module, especially in light of the comment above where this makes parse/serialize no longer inverse.

[dougwilson]

Please make sure the new code additions follow StandardJS style, as this module is trying to move there for uniformity (please don't reformat the entire code base in this PR, though, only the changed lines where applicable).

[wesleytodd]

The DX of this today is pretty bad:

res.setHeader('Set-Cookie', [
  cookie.serialize('one', '1'),
  cookie.serialize('two', '2'),
  cookie.serialize('three', '3'),
])

The new way would be a significant improvement to the DX:

res.setHeader('Set-Cookie', cookie.serialize({
  one: 1,
  two: 2,
  three: 3
}))

To me this is for sure worth the addition.

[dougwilson]

The DX of this today is pretty bad

That is because that is why the "cookies" module exists for better DX... otherwise we wouldn't have two modules, lol. This is meant to be very low level simple API from a complexity stand point and cookies module delivers any easy to use friendly API.

[dougwilson]

The main DX issue is that the Node.js API does not work well with the set-cookie header in general. Your example of your three examples, the 1st would never work, as you'd only end up with the last cookie on the response. The second two would only ever "work" if you made sure to not set set-cookie header somewhere else in your code, as otherwise you'd override existing set cookies on the response.

The idea behind the jshttp modules is for them to be completely agnostic of HTTP framework, even the Node.js one, so just works on strings in general (and results in a string that is of a header, takes in a string that is of a http header) so that something else can piece that together into an API that makes sense for a framework. This is just to note that, to me, using a framework API as an argument point inside jshttp is beyond the scope of this org (but not of the org pillarjs).

But even then, my general experience with modules that take in an object seems to be a mixed bag, in that one person expects it to be only own-enumerable properties (like this PR), but others feel is should include all enumerable properties (like for … in).

[wesleytodd]

That is because that is why the "cookies" module exists for better DX

I think this is a weak argument for not improving the api of this package. Also, IMO, the cookies package is a very different use case, and I have often wanted this api and not the one offered by cookies.

Your example of your three examples, the 1st would never work, as you'd only end up with the last cookie on the response.

" If this header already exists in the to-be-sent headers, its value will be replaced."

Yep you are correct, my bad. I don't typically use the lower level api here and miss-read the docs the first time. That said, the second example is also less optimal than what this provides. (edited my comment so I don't miss-lead folks who read this in the future)

The idea behind the jshttp modules is for them to be completely agnostic of HTTP framework, even the Node.js one

I don't think this PR goes against that. This is a general use api, and serializing multiple set cookie headers at once is a valid use case no matter where you are using it.

But even then, my general experience with modules that take in an object seems to be a mixed bag, in that one person expects it to be only own-enumerable properties (like this PR), but others feel is should include all enumerable properties (like for … in).

I think that as long as we document which way it behaves we are all good on this front. Do you disagree?

[dougwilson]

I think this is a weak argument for not improving the api of this package.

This is where I disagree. Every one is created with an API in mind, and following the idea of micro module vs macro modules I think that this is a very valid argument. These modules in jshttp are suppose to be single purpose with all header-based modules only providing two exports: a parse and a serlailize. This adds a third export, making a different way to serialize that just would introduce more complexirty to, based on this PR itself, save the user from 4-5 LOC, but bringing up the arguments of for ... in vs Object.keys.

Also, IMO, the cookies is a very different use case, and I have often wanted this api and not the one offered by cookies.

Sure, though why not a third module for this use case? What is the specific use case, if you don't mind? The examples you had here do not actually seem like complete use-cases, unless I'm missing something.

That said, the second example is also less optimal than what this provides.

That's right, and the third example is also less than optimal, as you're not appending to the set-cookie header, but using the Node.js progressive API, which makes it a non-progressive header addition.

I don't think this PR goes against that. This is a general use api, and serializing multiple set cookie headers at once is a valid use case no matter where you are using it.

I think it does, which is the argument I'm making. See the first and second responses in this comment for them :)

I think that as long as we document which way it behaves we are all good on this front. Do you disagree?

Yes, I definately do, because I know how it turns out every time. And even then, you're saying that we should add this because it makes it easier... but only for the set of people who want to enumerate it like this module does. For the others, you're saying they still need to do it on their own, which they will rightfully say is unfair and this module should make it easy for them, too. They will argue why does this module built in Object.keys but not for … in. And for that matter, why not support for giving Set, Map, etc. when those are "superior" dictionary objects.

[wesleytodd]

why not a third module for this use case?

I personally am fine with this approach, and I am generally a big fan of small focused modules. But I think being pragmatic is an important quality when designing software. I think this PR is an example of pragmatic design, in that many (not all) use cases would be served by adding this and reduce surface in user applications for bugs. For example, if we decide on one approach to the for ... in vs Object.keys question it prevents users from having to make that decision and doing it wrong since they are presumably less informed on this problem.

And for that matter, why not support for giving Set, Map, etc. when those are "superior" dictionary objects.

I actually think that would be a good idea lol. I see your point though.

That's right, and the third example is also less than optimal, as you're not appending to the set-cookie header, but using the Node.js progressive API, which makes it a non-progressive header addition.

This is exactly the kind of topic I think we should cover in the Web Server Frameworks Team. I will open an issue to see if there is a good way we can improve the DX here!

---

@ryhinchey I will hold off on more comments pending your thoughts as the OP. What do you think about us creating a higher level package for this? Is that reasonable to you?

[dougwilson]

I think this PR is an example of pragmatic design, in that many (not all) use cases would be served by adding this and reduce surface in user applications for bugs. For example, if we decide on one approach to the for ... in vs Object.keys question it prevents users from having to make that decision and doing it wrong since they are presumably less informed on this problem.

Yea, I get where you're coming from, but especially with the whole prototype pollution stuff going around the ecosystem, it is becoming more apparent that iterating through objects is not an easy thing to get done, and even specific docs doesn't seem to help. I got an email report of a security issue from one of the expressjs modules that I am going to get posted to our security channel later today that actually is an issue with exactly this type of design. The module in question tried to have an API with easier developer DX with strings vs objects and users are combining it with other API surface areas in a way where the iteration over the object causes a security issue. It's not a security issue in the expressjs module per-se, but rather the lack of reading the docs fully and making assumptions and combining them in a bad way.

[ryhinchey]

The idea behind the jshttp modules is for them to be completely agnostic of HTTP framework, even the Node.js one, so just works on strings in general (and results in a string that is of a header, takes in a string that is of a http header) so that something else can piece that together into an API that makes sense for a framework. This is just to note that, to me, using a framework API as an argument point inside jshttp is beyond the scope of this org (but not of the org pillarjs).

This is a very helpful explanation of what types of things go under jshttp. thank you!

[ryhinchey]

@ryhinchey I will hold off on more comments pending your thoughts as the OP. What do you think about us creating a higher level package for this? Is that reasonable to you?

@wesleytodd since the implementation is only a few lines of code I personally don't think it makes sense to have a separate module just for creating multiple Set-Cookie headers. Also, the parse/serialize simplicity of all of the header modules is nice and I wouldn't want to break that here.

If we were to identify situations where multiple headers makes sense (likeSet-Cookie) and create a module to support all of those, I think that could be helpful. Is that what you were thinking?

[wesleytodd]

I got an email report of a security issue from one of the expressjs modules that I am going to get posted to our security channel later today that actually is an issue with exactly this type of design

Interesting, looking forward to seeing the report. I for sure agree we need to prioritize security over DX. I will hold here off for now to see what comes of that and the conversation about supporting appending headers in core (which I think might also be a way around this in a more suitable way).

If we were to identify situations where multiple headers makes sense (likeSet-Cookie) and create a module to support all of those, I think that could be helpful. Is that what you were thinking?

I think this belongs in core. I opened this to kick that conversation off: nodejs/web-server-frameworks#32

[dougwilson]

I will hold here off for now to see what comes of that and the conversation about supporting appending headers in core

Yep, I saw you open that and look forward to hearing a resounding "yes we should"! Lol. Express provides res.append if you are in our ecosystem, and likely the other frameworks have something as well.

[wesleytodd]

They do, I reached out to the restify team internally and hey have almost the exact same thing! This is exactly what I had hoped would come out of the "take over low level http packages" would result in, so hopefully folks like it.

[samkahchiin]

May I know what is the current status for this PR?
I am waiting for this feature to be released for quite some times.

[rakspac]

Hi, I am already using this lib and came across the exact same requirement to support the multiple cookies.
Any timeline when can we expect this functionality?

Also, kindly suggest if there are any available workarounds till then.

[ryhinchey]

closing this PR as it does not make sense to land in this cookie module