-
Notifications
You must be signed in to change notification settings - Fork 146
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.
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
add serializeObject export to serialize multiple cookies at a time #99
Conversation
Thank you!
Can you elaborate on this point? What is changed between this PR and that and why? |
b4b3130
to
1e898f3
Compare
@dougwilson PR #47 modifies // current functionality
cookie.serialize('foo', 'bar', {});
// PR addition
cookie.serialize({foo: 'bar'}, {}); It also adds a function Looking at the PR, the parameters A comment mentioned This PR keeps 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 😄 |
Because 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. |
@jamesarosen my thinking was that folks might want to have multiple |
I keep thinking that
is not valid (though I really wish it were). |
1e898f3
to
3130022
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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....
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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. |
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. |
The main DX issue is that the Node.js API does not work well with the 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 |
I think this is a weak argument for not improving the api of this package. Also, IMO, the
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)
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 that as long as we document which way it behaves we are all good on this front. Do you disagree? |
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
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'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 think it does, which is the argument I'm making. See the first and second responses in this comment for them :)
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 |
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
I actually think that would be a good idea lol. I see your point though.
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? |
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. |
This is a very helpful explanation of what types of things go under jshttp. thank 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 If we were to identify situations where multiple headers makes sense (like |
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).
I think this belongs in core. I opened this to kick that conversation off: nodejs/web-server-frameworks#32 |
Yep, I saw you open that and look forward to hearing a resounding "yes we should"! Lol. Express provides |
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. |
May I know what is the current status for this PR? |
Hi, I am already using this lib and came across the exact same requirement to support the multiple cookies. Also, kindly suggest if there are any available workarounds till then. |
closing this PR as it does not make sense to land in this cookie module |
Proposal for issue #30
Alternative approach from PR #47