Skip to content

Updated docs for getServerSideProps and getStaticProps return values#33577

Merged
kodiakhq[bot] merged 6 commits intovercel:canaryfrom
chozzz:fix-33576
Jan 26, 2022
Merged

Updated docs for getServerSideProps and getStaticProps return values#33577
kodiakhq[bot] merged 6 commits intovercel:canaryfrom
chozzz:fix-33576

Conversation

@chozzz
Copy link
Copy Markdown
Contributor

@chozzz chozzz commented Jan 23, 2022 •
edited
Loading

This fixes the documentation for getServerSideProps and getStaticProps which mislead the defined type.

Current screenshot taken from nextjs doc

image

However, the declared type for these types are using union type which represents any of the following instead of optional.

Declared types;
GetStaticPropsResult
image

GetServerSidePropsResult
image

This PR updates the documentation for their return values;

Before: The ... function should return an object with the following optional properties:
After: The ... function should return object with any one of the following properties:

Bug

  • Related issues linked using fixes #number
  • Integration tests added
  • Errors have helpful link attached, see contributing.md

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Integration tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.
  • Errors have helpful link attached, see contributing.md

Documentation / Examples

  • Make sure the linting passes by running yarn lint

leerob
leerob reviewed Jan 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@leerob leerob left a comment

Choose a reason for hiding this comment

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

Would you mind updating getStaticPaths as well? Thank you!

https://github.com/vercel/next.js/blob/canary/docs/api-reference/data-fetching/get-static-paths.md#getstaticpaths-return-values

@chozzz
Copy link
Copy Markdown
Contributor Author

chozzz commented Jan 24, 2022

Would you mind updating getStaticPaths as well? Thank you!

https://github.com/vercel/next.js/blob/canary/docs/api-reference/data-fetching/get-static-paths.md#getstaticpaths-return-values

@leerob getStaticPaths seems to be already in order based on its type. Can you elaborate more?

@chozzz chozzz requested a review from leerob January 24, 2022 00:32
@leerob
Copy link
Copy Markdown
Contributor

leerob commented Jan 24, 2022

It says "following required properties:" which I feel is misleading. "Any of the following" seems to make more sense, because there are multiple types of fallback - what do you think?

balazsorban44
balazsorban44 suggested changes Jan 24, 2022
View reviewed changes
Comment thread docs/api-reference/data-fetching/get-static-props.md Outdated
@balazsorban44 balazsorban44 linked an issue Jan 24, 2022 that may be closed by this pull request
clarify possible getServerSideProps return values #33576
Closed
@chozzz
Copy link
Copy Markdown
Contributor Author

chozzz commented Jan 24, 2022

It says "following required properties:" which I feel is misleading. "Any of the following" seems to make more sense, because there are multiple types of fallback - what do you think?

I think the following required properties is the correct one.
getStaticProps object need both paths and fallback as per the declared type

@chozzz chozzz dismissed a stale review via b6db0e3 January 24, 2022 04:49
@chozzz @balazsorban44
Update docs/api-reference/data-fetching/get-static-props.md
b6db0e3 
Co-authored-by: Balázs Orbán <info@balazsorban.com>
@chozzz chozzz requested a review from balazsorban44 January 24, 2022 04:49
@chozzz chozzz mentioned this pull request Jan 24, 2022
Closed
balazsorban44
balazsorban44 reviewed Jan 24, 2022
View reviewed changes
Comment thread docs/api-reference/data-fetching/get-static-props.md Outdated
balazsorban44
balazsorban44 suggested changes Jan 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@balazsorban44 balazsorban44 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left a comment on getStaticProps.

To clarify getStaticPaths, paths is always required, but fallback is optional and will default to fallback: false

I was wrong, both are actually required, you were right, so the getStaticPaths docs looks good.

next.js/packages/next/build/utils.ts

Lines 694 to 704 in 0fb1183

if (
!(
typeof staticPathsResult.fallback === 'boolean' ||
staticPathsResult.fallback === 'blocking'
)
) {
throw new Error(
`The \`fallback\` key must be returned from getStaticPaths in ${page}.\n` +
expectedReturnVal
)
}

@chozzz
Copy link
Copy Markdown
Contributor Author

chozzz commented Jan 25, 2022

@balazsorban44 Updated the wording on getStaticProps, please re-review.

@chozzz chozzz requested a review from balazsorban44 January 25, 2022 15:13
@kodiakhq kodiakhq Bot merged commit d055fee into vercel:canary Jan 26, 2022
This was referenced Jan 27, 2022
Open
Open
Open
Open
Open
Open
Open
Open
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@huozhi huozhi Awaiting requested review from huozhi

@ijjk ijjk Awaiting requested review from ijjk

@padmaia padmaia Awaiting requested review from padmaia

@shuding shuding Awaiting requested review from shuding

@styfle styfle Awaiting requested review from styfle

@timneutkens timneutkens Awaiting requested review from timneutkens

2 more reviewers

@leerob leerob leerob approved these changes

@balazsorban44 balazsorban44 balazsorban44 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

clarify possible getServerSideProps return values

5 participants

@chozzz @leerob @balazsorban44 @ijjk @vadi-toptal