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

Update getServerSideProps docs with req.cookie note #29457

Merged
merged 5 commits into from
Sep 30, 2021

Conversation

vitorbal
Copy link
Contributor

@vitorbal vitorbal commented Sep 28, 2021

Documentation / Examples

Add a note to the getServerSideProps docs that context.req contains a context.req.cookies attribute.

I based the copy off of the one from https://nextjs.org/docs/api-routes/api-middlewares.

Checklist:

  • Make sure the linting passes

@vitorbal vitorbal force-pushed the getServerSideProps-cookie-docs branch from a73295b to 6214bfa Compare September 29, 2021 11:34
Comment on lines 730 to 734
### Built-in `context.req` parsing

To improve the developer experience, the `context.req` request object received by `getServerSideProps` is pre-parsed to include some additional helpful properties. Those properties are:

- `context.req.cookies` - An object containing the cookies sent by the request. Defaults to `{}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@styfle @leerob @molebox took another stab at this, with a dedicated sub-section for context.req built-in parsing. What do you all think?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I was thinking there were more helpers than just cookies but then I checked the type and its just cookies like you mentioned

req: IncomingMessage & {
cookies: NextApiRequestCookies
}
res: ServerResponse

And it looks like res doesn't have any helpers so maybe its best to go back to your original commit where it was mentioned inline.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe we discovered a strange behavior where getServerSideProps() differs to much from API Routes and we should try to use the same helpers for both 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I don't think it hurts to have this extra section, even if it just talks about 1 helper. Nice for google foo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @molebox, I like that we'd be able to deeplink to this section. Additionally, this makes it easier to expand the list in the future if we do want to get to parity with the API route helpers

docs/basic-features/data-fetching.md Outdated Show resolved Hide resolved
Comment on lines 730 to 734
### Built-in `context.req` parsing

To improve the developer experience, the `context.req` request object received by `getServerSideProps` is pre-parsed to include some additional helpful properties. Those properties are:

- `context.req.cookies` - An object containing the cookies sent by the request. Defaults to `{}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I don't think it hurts to have this extra section, even if it just talks about 1 helper. Nice for google foo

Co-authored-by: Rich Haines <hello@richardhaines.dev>
molebox
molebox previously approved these changes Sep 30, 2021
Copy link
Collaborator

@molebox molebox left a comment

Choose a reason for hiding this comment

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

seal-of-approval

@vitorbal
Copy link
Contributor Author

@molebox do you have the super powers necessary to approve running the workflows?

@molebox
Copy link
Collaborator

molebox commented Sep 30, 2021

@Vitorba
i-got-the-power
l

Tweak the wording of the new getServerSideProps middleware section so it matches closer to the one in https://nextjs.org/docs/api-routes/api-middlewares.

Co-authored-by: JJ Kasper <jj@jjsweb.site>
@vitorbal
Copy link
Contributor Author

@ijjk thanks, good tweaks! Added 😃

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Thanks!

@kodiakhq kodiakhq bot merged commit 2f26fb4 into vercel:canary Sep 30, 2021
@vitorbal vitorbal deleted the getServerSideProps-cookie-docs branch September 30, 2021 15:47
@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
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.

4 participants