Skip to content

fix(astro): Protect component prop types #3846

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.

Sign up for GitHub

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

Merged
wobsoriano merged 3 commits into from
Jul 31, 2024
Merged

fix(astro): Protect component prop types #3846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b52281d
fix(astro): Protect component prop types
wobsoriano Jul 30, 2024
f6e5038
chore(astro): Include Protect component prop types inside control com…
wobsoriano Jul 30, 2024
6c66ae7
chore(astro): Add changeset
wobsoriano Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/friendly-bags-whisper.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@clerk/astro": patch
---

Fixed a bug where the `<Protect />` component would not validate any properties passed
29 changes: 27 additions & 2 deletions packages/astro/src/astro-components/control/Protect.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,31 @@
---
import { ProtectComponentDefaultProps } from "../../types";
type Props = ProtectComponentDefaultProps
import type {
CheckAuthorizationWithCustomPermissions,
OrganizationCustomPermissionKey,
OrganizationCustomRoleKey,
} from '@clerk/types';

type Props =
| {
condition?: never;
role: OrganizationCustomRoleKey;
permission?: never;
}
| {
condition?: never;
role?: never;
permission: OrganizationCustomPermissionKey;
}
| {
condition: (has: CheckAuthorizationWithCustomPermissions) => boolean;
role?: never;
permission?: never;
}
| {
condition?: never;
role?: never;
permission?: never;
};

const { has, userId } = Astro.locals.auth();
const isUnauthorized =
Expand Down
31 changes: 28 additions & 3 deletions packages/astro/src/react/controlComponents.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import type { CheckAuthorizationWithCustomPermissions, HandleOAuthCallbackParams } from '@clerk/types';
import type {
CheckAuthorizationWithCustomPermissions,
HandleOAuthCallbackParams,
OrganizationCustomPermissionKey,
OrganizationCustomRoleKey,
} from '@clerk/types';
import { computed } from 'nanostores';
import type { PropsWithChildren } from 'react';
import React, { useEffect, useState } from 'react';

import { $csrState } from '../stores/internal';
import type { ProtectComponentDefaultProps } from '../types';
import { useAuth } from './hooks';
import type { WithClerkProp } from './utils';
import { withClerk } from './utils';
Expand Down Expand Up @@ -70,7 +74,28 @@ export const ClerkLoading = ({ children }: React.PropsWithChildren): JSX.Element
};

export type ProtectProps = React.PropsWithChildren<
ProtectComponentDefaultProps & {
(
| {
condition?: never;
role: OrganizationCustomRoleKey;
permission?: never;
}
| {
condition?: never;
role?: never;
permission: OrganizationCustomPermissionKey;
}
| {
condition: (has: CheckAuthorizationWithCustomPermissions) => boolean;
role?: never;
permission?: never;
}
| {
condition?: never;
role?: never;
permission?: never;
}
) & {
Comment on lines +77 to +98
Copy link
Member Author

@wobsoriano wobsoriano Jul 30, 2024

Choose a reason for hiding this comment

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

We need to separate the prop types of the Astro component and React component since the Astro one has no build step and imported types will not be resolvable.

Copy link
Member

@alexcarpenter alexcarpenter Jul 31, 2024

Choose a reason for hiding this comment

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

Would the astro prop types need fallback: never; added?

Copy link
Member Author

@wobsoriano wobsoriano Jul 31, 2024

Choose a reason for hiding this comment

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

If the purpose of adding fallback: never is to show an error whenever unexpected props are used in Astro <Protect />, we don't need it it since it should already show type errors for random props

Also just FYI, Astro does not support typed slots

fallback?: React.ReactNode;
}
>;
Expand Down
38 changes: 2 additions & 36 deletions packages/astro/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,4 @@
import type {
CheckAuthorizationWithCustomPermissions,
ClerkOptions,
MultiDomainAndOrProxyPrimitives,
OrganizationCustomPermissionKey,
OrganizationCustomRoleKey,
Without,
} from '@clerk/types';
import type { ClerkOptions, MultiDomainAndOrProxyPrimitives, Without } from '@clerk/types';

type AstroClerkUpdateOptions = Pick<ClerkOptions, 'appearance' | 'localization'>;

Expand All @@ -32,31 +25,4 @@ declare global {
}
}

type ProtectComponentDefaultProps =
| {
condition?: never;
role: OrganizationCustomRoleKey;
permission?: never;
}
| {
condition?: never;
role?: never;
permission: OrganizationCustomPermissionKey;
}
| {
condition: (has: CheckAuthorizationWithCustomPermissions) => boolean;
role?: never;
permission?: never;
}
| {
condition?: never;
role?: never;
permission?: never;
};

export type {
AstroClerkUpdateOptions,
AstroClerkIntegrationParams,
AstroClerkCreateInstanceParams,
ProtectComponentDefaultProps,
};
export type { AstroClerkUpdateOptions, AstroClerkIntegrationParams, AstroClerkCreateInstanceParams };