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

chore: [app dir bootstrapping 4] check nullability of navigation hook return values #12005

Merged
merged 5 commits into from
Oct 20, 2023

Conversation

grzpab
Copy link
Contributor

@grzpab grzpab commented Oct 19, 2023

What does this PR do?

This PR enforces nullability checks for return values from useParams and useSearchParams.
If a page has neither getServerSideProps not getInitialProps, Next.js will optimize it statically. This makes the usePathname and useSearchParams equal null until the router becomes available during hydration.

If you add the app directory to the web directory and run yarn build, the types will be updated by Next.js builder/plugin and the projects will then include the compat type definitions provided by Next.js.

This mechanism is coded here: https://github.com/vercel/next.js/blob/524b31513a58e58e15862ac8aa3f27da8a47a267/packages/next/src/lib/typescript/writeAppTypeDeclarations.ts#L54

The example compat type definition is here: https://github.com/vercel/next.js/blob/524b31513a58e58e15862ac8aa3f27da8a47a267/packages/next/navigation-types/compat/navigation.d.ts

Requirement/Documentation

https://nextjs.org/docs/app/api-reference/functions/use-pathname
https://nextjs.org/docs/app/api-reference/functions/use-search-params
https://nextjs.org/docs/pages/building-your-application/rendering/automatic-static-optimization

Type of change

  • Chore (refactoring code, technical debt, workflow improvements)

How should this be tested?

The app should work exactly as worked before.

Since nullability issues (TypeErrors) have not been observed before, this change should only satisfy the TS compiler.

Caveats:

  1. The prerendered pages doesn't have the useSearchParams and usePathname return values on the prerender. Other pages don't have these values on the first render. The code should not allow:
    a) running write operation on first render (with e.g. useEffect) that use values from these hooks.
    b) running read operations unconditionally based on the values from these hooks.

One idea was to wrap each component with a HOC that:

  1. returns null when useSearchParams or usePathname are null
  2. otherwise, it passes non-empty searchParams and pathname as props to the wrapped component.
    One problem is possible flickering stemming from a lot of components appearing when these two values become available.

Mandatory Tasks

  • Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Checklist

  • I haven't checked if my PR needs changes to the documentation
  • I haven't checked if my changes generate no new warnings
  • I haven't added tests that prove my fix is effective or that my feature works

@vercel
Copy link

vercel bot commented Oct 19, 2023

@grzpab is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Oct 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 20, 2023 0:23am

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link to collect XP and win prizes!

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@grzpab grzpab marked this pull request as ready for review October 20, 2023 12:30
@PeerRich PeerRich added the Medium priority Created by Linear-GitHub Sync label Oct 20, 2023
@grzpab grzpab changed the title chore: check nullability of navigation hook return values chore: [app dir bootstrapping 4] check nullability of navigation hook return values Oct 20, 2023
setLoadMore(true);
}
setMembers(members.concat(membersFetch));
setLoadMore(membersFetch.length >= limit);
Copy link
Member

Choose a reason for hiding this comment

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

nice

Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Nice. Big change. Will test locally before approving. Thanks again @grzpab 🙏🏽

Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Code LGTM. Tested locally and found no issues. Checks are passing. I think it's safe to merge and test on dev and QA.

@zomars zomars merged commit 39cfe18 into calcom:main Oct 20, 2023
29 of 36 checks passed
@zomars zomars deleted the gpp/nullability1 branch October 20, 2023 23:47
hbjORbj pushed a commit to codemod-com/cal.com-demo that referenced this pull request Nov 28, 2023
hbjORbj pushed a commit to codemod-com/cal.com-demo that referenced this pull request Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Medium priority Created by Linear-GitHub Sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants