Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

implement module #1

Merged
merged 3 commits into from
Aug 9, 2018
Merged

implement module #1

merged 3 commits into from
Aug 9, 2018

Conversation

Qard
Copy link
Contributor

@Qard Qard commented Jul 30, 2018

This is an initial implementation of a set-cookie parser and serializer.

@Qard Qard self-assigned this Jul 30, 2018
Copy link

@watson watson left a comment

Choose a reason for hiding this comment

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

Looks good! Left a few comments. Btw, when adding .travis.yml, I recommend adding Node.js v4 and v9 to the matrix to match Elastic APM

class SetCookie {
constructor (input, options) {
if (Array.isArray(input)) {
return input.map(item => new SetCookie(item, options))
Copy link

Choose a reason for hiding this comment

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

Will this work if people do new SetCookie(array)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it will return an array of SetCookie instances.

Copy link

Choose a reason for hiding this comment

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

Will return statements not be ignored inside of a constructor?

Copy link
Contributor Author

@Qard Qard Aug 1, 2018

Choose a reason for hiding this comment

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

Nope, if you return in a constructor, the return value of the new Constructor will be whatever was returned. 😸

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests prove this already, by the way. 😸

index.js Outdated
maxAge: undefined,
domain: undefined,
path: undefined,
secure: undefined,
Copy link

Choose a reason for hiding this comment

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

Consider setting this default value to false

index.js Outdated
domain: undefined,
path: undefined,
secure: undefined,
httpOnly: undefined,
Copy link

Choose a reason for hiding this comment

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

Consider setting this default value to false

index.js Outdated
}

data = {
key: this[decode](pair[0]),
Copy link

Choose a reason for hiding this comment

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

From https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie:

  • __Secure- prefix: Cookies with a name starting with __Secure- (dash is part of the prefix) must be set with the secure flag and must be from a secure page (HTTPS).
  • __Host- prefix: Cookies with a name starting with __Host- must be set with the secure flag, must be from a secure page (HTTPS), must not have a domain specified (and therefore aren't sent to subdomains) and the path must be "/".

Copy link

Choose a reason for hiding this comment

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

Did you consider this?

@Qard
Copy link
Contributor Author

Qard commented Aug 7, 2018

@watson Can I get another review of this?

@Qard Qard merged commit a01ebc2 into master Aug 9, 2018
@Qard Qard deleted the implementation branch August 9, 2018 18:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants