Skip to content

fix(useDocumentInfo): Add explicit null to types #9030

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

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

eranhirsch
Copy link

@eranhirsch eranhirsch commented Nov 5, 2024

What?

Added | null to the type for context props initialized to null.

Why?

For code bases using a more strict typescript configuration, null and undefined (and optional fields) are different types and require handling separately. The states for these three context fields are all initialized to null, but their type doesn't include that. This caused runtime errors in our codebase.

How?

Modified the type, both in the context type, and in the react setters. Notice that this change doesn't change anything in the runtime behaviour of the code, just types.

@eranhirsch eranhirsch changed the base branch from beta to main November 5, 2024 09:52
@DanRibbens
Copy link
Contributor

I think we need to wait and review this change after the RSC refactor PR because the DocumentInfoProvider is undering major changes right now.

@eranhirsch
Copy link
Author

This is for the v2 branch. Are you going to push RSC to the v2 branch?

Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

This is for the v2 branch. Are you going to push RSC to the v2 branch?

I missed that the target branch is main on this. Unfortunately we need to avoid breaking changes so I don't think we can merge this as-is. Even if the types should technically have a union null it could cause existing 2.x projects to not build by changing it now.

The only options are to migrate to 3.x or we would have to a nasty option flag or make a separate function for useDocumentInfo that could make this type change only for the new properties.

@eranhirsch
Copy link
Author

Can this be released as a minor version (not a patch version) so that users are informed about a possible breakage? This doesn't cause new runtime issues; it simply surfaces potential issues that already exist in the system. AFAIK V3 is still in beta, so it's not an option for us to migrate.

@DanRibbens
Copy link
Contributor

DanRibbens commented Nov 6, 2024

Can this be released as a minor version (not a patch version) so that users are informed about a possible breakage?

No, following semantic versioning, we can only make breaking changes in major versions. We have been reprimanded by the community for this in the past.

You could add a custom provider in your own config that simply gets the useDocumentInfo properties and passes them through, typed the way you want them.

@tyteen4a03
Copy link
Contributor

we would have to a nasty option flag

I think we should normalize the use of future flags though I understand if this isn't on the team's appetite for v2.

@eranhirsch
Copy link
Author

eranhirsch commented Nov 7, 2024

import { useDocumentInfo as useDocumentInfo_ORIGINAL } from 'payload/components/utilities';

type DocumentInfo = ReturnType<typeof useDocumentInfo_ORIGINAL>;

const ALL_MISTYPED_PROPS = [
  'versions',
  'publishedDoc',
  'unpublishedVersions',
] as const satisfies (keyof DocumentInfo)[];

type MistypedProps = (typeof ALL_MISTYPED_PROPS)[number];

export const useDocumentInfo: (...args: Parameters<typeof useDocumentInfo_ORIGINAL>) => Omit<
  DocumentInfo,
  MistypedProps
> & {
  [P in MistypedProps]?: Required<DocumentInfo>[P] | null;
} = useDocumentInfo_ORIGINAL;

@denolfe denolfe added the v2 label Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants