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

BREAKING(http): cookie headers as params #1041

Merged
merged 10 commits into from
Sep 13, 2021
Merged

BREAKING(http): cookie headers as params #1041

merged 10 commits into from
Sep 13, 2021

Conversation

timreichen
Copy link
Contributor

Change cookie functions getCookies, setCookie and deleteCookie params to Headers instance instead of object with (optional) Headers instance property.
The former param was so a response can directly be passed. Since the functions only require a Headers instance to manipulate, that should be passed directly.

before

setCookie(response,  { name: "foo", value: "bar" })

after

setCookie(response.headers,  { name: "foo", value: "bar" })

If any of the functions wanted to be used on a Headers instance directly, it had to be wrapped:

before

const headers = new Headers()
setCookie({ headers },  { name: "foo", value: "bar" })

after

const headers = new Headers()
setCookie(headers,  { name: "foo", value: "bar" })

Since the headers property in setCookie was optional, this allowed to pass an empty object in which a headers property would be initiated, manipulated, which seems avoidable.

before

cookie.setCookie({}, { name: "foo", value: "bar" }) // weird

after
not possible

http/cookie.ts Outdated
@@ -164,8 +164,8 @@ function validateDomain(domain: string): void {
* Parse the cookies of the Server Request
* @param req An object which has a `headers` property
Copy link
Contributor

@cmorten cmorten Aug 31, 2021

Choose a reason for hiding this comment

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

Suggested change
* @param req An object which has a `headers` property
* @param {Headers} headers The headers instance to get cookies from.

Need to take care to keep the jsdocs in line with changes 🙂

Sim. applies to other method interface changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

By way of "improve what you touch", consider also adding the @return {Object} … jsdoc for these methods as well to add clarity to what is returned for users of the Deno docs.

http/cookie.ts Outdated
@@ -195,16 +196,13 @@ export function getCookies(req: { headers: Headers }): Record<string, string> {
* httpOnly: true, secure: true, maxAge: 2, domain: "deno.land" });
Copy link
Contributor

Choose a reason for hiding this comment

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

See also the @param definitions for this method.

http/cookie.ts Outdated
@@ -219,11 +217,11 @@ export function setCookie(res: { headers?: Headers }, cookie: Cookie): void {
* deleteCookie(res,'foo', {domain:'deno.land'});
Copy link
Contributor

Choose a reason for hiding this comment

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

Sim.

@timreichen
Copy link
Contributor Author

@cmorten
I added a @return and updated the @param. But I think the jsdocs probably need an overdo for each function in this file. Is there a style guide on how to do jsdocs?

  • Dot at the end of a single phrase (both present in std)
  • @returnor @returns (both present in std)
  • examples with import statement or not (both present in std)

Probably best to open a separate PR for this.

@cmorten
Copy link
Contributor

cmorten commented Aug 31, 2021

Is there a style guide on how to do jsdocs?

Only what is currently in https://deno.land/manual/contributing/style_guide#use-jsdoc-for-exported-symbols, though not sure it’s been kept to strictly.

  • @returnor @returns (both present in std)

I think the singular simply because that is what doc.deno seems to support with syntax highlighting.

  • examples with import statement or not (both present in std)

I believe deno test —doc requires the example code block to be fully functional/self-contained. This might require use of an import statement. (Some older example blocks aren’t fenced, but instead indented, and therefore not tested)

Probably best to open a separate PR for this.

We should make sure the jsdocs are at least consistent with the implementation, anything more perhaps could be raised as a separate issue and PR.

@timreichen
Copy link
Contributor Author

@cmorten I updated readme and jsdoc and tests pass now.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

I think this now makes sense because we switch to the new http/server module.

@timreichen Thank your for your suggestion and contribution!

Thanks @cmorten for reviewing.

@kt3k kt3k merged commit 63f22aa into denoland:main Sep 13, 2021
@timreichen timreichen deleted the BREAKING(http)--cookie-header-params branch October 7, 2021 20:48
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.

None yet

3 participants