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

Updated docs for getServerSideProps and getStaticProps return values #33577

Merged
merged 6 commits into from
Jan 26, 2022

Conversation

chozzz
Copy link
Contributor

@chozzz chozzz commented Jan 23, 2022

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

Copy link
Member

@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.

@chozzz
Copy link
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
Member

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 linked an issue Jan 24, 2022 that may be closed by this pull request
@chozzz
Copy link
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
Co-authored-by: Balázs Orbán <info@balazsorban.com>
Copy link
Member

@balazsorban44 balazsorban44 left a comment

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.

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
Contributor Author

chozzz commented Jan 25, 2022

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

@kodiakhq kodiakhq bot merged commit d055fee into vercel:canary Jan 26, 2022
natew pushed a commit to natew/next.js that referenced this pull request Feb 16, 2022
…ercel#33577)

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

Current screenshot taken from [nextjs doc](https://nextjs.org/docs/api-reference/data-fetching/get-server-side-props)

![image](https://user-images.githubusercontent.com/5223310/150689799-5da3c1b6-61ae-4455-8970-8c96f025474a.png)

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

Declared types;
[GetStaticPropsResult](https://github.com/vercel/next.js/blob/canary/packages/next/types/index.d.ts#L107)
![image](https://user-images.githubusercontent.com/5223310/150689919-5f3cf8f8-60d3-48e4-9412-993e2ead0634.png)

[GetServerSidePropsResult](https://github.com/vercel/next.js/blob/canary/packages/next/types/index.d.ts#L160)
![image](https://user-images.githubusercontent.com/5223310/150689931-16fe3bef-eb4e-41cd-b087-98c3282d599d.png)

---

### 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

- [x] 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
- [x] 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

- [x] Make sure the linting passes by running `yarn lint`


Co-authored-by: Vadi Taslim <70912283+vaditaslim@users.noreply.github.com>
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clarify possible getServerSideProps return values
5 participants