Skip to content
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

http : Add cookie module #338

Merged
merged 13 commits into from Apr 24, 2019

Conversation

3 participants
@zekth
Copy link
Contributor

commented Apr 13, 2019

cookie is now part of ServerRequest and can be accessed by ServerRequest.cookie
Resolve #334

zekth added some commits Apr 13, 2019

@zekth zekth changed the title http/server : add cookie helper on request http/server : add cookie property on ServerRequest Apr 13, 2019

req.headers = new Headers();
assertEquals(req.cookie, {});
req.headers = new Headers();
req.headers.set("Cookie", "foo=bar");
assertEquals(req.cookie, { foo: "bar" });
assertEquals(req.cookie, {});

This comment has been minimized.

Copy link
@ry

ry Apr 13, 2019

Contributor

This seems wrong?

This comment has been minimized.

Copy link
@zekth

zekth Apr 13, 2019

Author Contributor

To read this yes it can feel wrong but it's just to show the cache handling.
At first the ServerRequest is created and the headers are set. After this the req.cookie is called, which will try to parse the headers to find cookies. It can't find anything so {} is stored in _cookie. Next headers is set to foo=bar and we try to recall req.cookie and it returns {} which is right because we have already called it and it's cached. The only way to get foo=bar is to recreate a ServerRequest like shown after in the test. Why do this? Because you don't set request cookie header when you try to access it, it's already set by the client request. It was just to cache the cookie parsing, especially in case of web server frameworks and middleware which can call this a lot while processing a request.

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Another possibility:
Replace get cookie with function cookie(reload: boolean)
If reload is true it will parse again the headers to set _cookie value. IMO it's not really necessary as explained into the review and i think it's easier to call req.headers.cookie instead of req.headers.cookie()

Show resolved Hide resolved http/server.ts Outdated
Show resolved Hide resolved http/server.ts Outdated

zekth added some commits Apr 14, 2019

Show resolved Hide resolved http/server.ts Outdated

zekth added some commits Apr 14, 2019

Show resolved Hide resolved http/server.ts Outdated
Show resolved Hide resolved http/server.ts Outdated

zekth added some commits Apr 23, 2019

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Added the getCookie() method to force reloading as documented in the code.

@ry

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Can this moved into a separate module? I’d rather http/server.ts stay lean

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Can this moved into a separate module? I’d rather http/server.ts stay lean

Moving the ServerRequest class in a separate module so? Or you want to completely separate the cookie parsing from ServerRequest class? If so it might be interesting to have and ExtendedServerRequest which adds header parsing logic and so on. Your thoughts?

@ry

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

@zekth Ideally just a function.

import { getCookies } from "https://deno.land/std/http/cookies.ts"

getCookies(request);

Something like this?

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

@zekth Ideally just a function.

import { getCookies } from "https://deno.land/std/http/cookies.ts"

getCookies(request);

Something like this?

Ok, but do we still keep the cache handling of the cookies in the ServerRequest class? I think it would be a good addition for the future frameworks and middleware layers trying to access to cookies. The headers will just be parsed one time.

@ry

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Ok, but do we still keep the cache handling of the cookies in the ServerRequest class?

No. I think it's fine to keep this in the external modules. It's an extra import, but frameworks can make that easier for people. Many users of http/server.ts will not need cookies, if we can keep some functionality out we should.

@zekth zekth changed the title http/server : add cookie property on ServerRequest http : Add cookie module Apr 24, 2019

@ry

ry approved these changes Apr 24, 2019

Copy link
Contributor

left a comment

LGTM - nice

@ry ry merged commit 1d85447 into denoland:master Apr 24, 2019

5 checks passed

denoland.deno_std (1) Build #20190424.3 succeeded
Details
denoland.deno_std (1) (Linux) Linux succeeded
Details
denoland.deno_std (1) (Mac) Mac succeeded
Details
denoland.deno_std (1) (Windows) Windows succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@zekth zekth deleted the zekth:cookie-handling branch Apr 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.