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

docs: update useParams hook return statement #67384

Open
wants to merge 1 commit into
base: canary
Choose a base branch
from

Conversation

Kuboczoch
Copy link

Hi 👋🏻

The Change

I noticed that in one example of the useParams hook, there is an old-fashioned return statement that returns a react fragment instead of null. This step was part of the workaround from the typescript environment from 2016, which is now obsolete.

Recently, I researched why some people do it today and whether we should continue doing it in a react environment.
I created a document discussing it:
https://github.com/Kuboczoch/web-dev-best-practices/blob/master/react/conditional-rendering_return-react-fragment.md

tl;dr

While returning a React.Fragment instead of null in a React component was a workaround for TypeScript limitations in 2016, this is no longer necessary. Returning null is more efficient, makes the code more readable, is officially supported, and aligns with current best practices.

Why?

It helps to provide the best practice to other developers who are just starting with web development and are mindlessly copy-pasting the code. In this case, people do not need to research workarounds for TypeScript from six years ago.

@ijjk ijjk added the Documentation Related to Next.js' official documentation. label Jul 2, 2024
@ijjk
Copy link
Member

ijjk commented Jul 2, 2024

Allow CI Workflow Run

  • approve CI run for commit: aa5baea

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@@ -18,7 +18,7 @@ export default function ExampleClientComponent() {
// `params` -> { tag: 'shoes', item: 'nike-air-max-97' }
console.log(params)

return <></>
return null
Copy link
Contributor

@devjiwonchoi devjiwonchoi Jul 2, 2024

Choose a reason for hiding this comment

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

Since it is an example component, it seems like the fragment was used to abstract the rest code, not just returning null.

How about something like this?

Suggested change
return null
return <>{/* ...rest */}</>

Copy link
Author

Choose a reason for hiding this comment

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

My example was more about what happens when you don't return anything. I wanted to keep the functionality the same as for the previous docs.
In my real-life production code, we don't expect anything in the return statement anyway for this component, and the <></> react fragment is confusing for new developers.

But your change is good too!
Can we do it in the next pr?

Let me know what you think!

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return null
return '...'

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go with this! Will further use'...' throughout the examples instead of null or <></> 😄

Copy link
Contributor

@devjiwonchoi devjiwonchoi left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Related to Next.js' official documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants