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

feat(clerk-js): Use org:sys_domains:read for improved fine grain control of the UI #1896

Merged
merged 9 commits into from
Oct 31, 2023

Conversation

panteliselef
Copy link
Member

@panteliselef panteliselef commented Oct 16, 2023

Description

This PR makes usage of org:sys_domains:read a permission that allows users to see a list of domains. Previously we were wrapping the whole functionality under the manage permission.

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/remix
  • @clerk/types
  • @clerk/themes
  • @clerk/localizations
  • @clerk/clerk-expo
  • @clerk/backend
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/fastify
  • @clerk/chrome-extension
  • gatsby-plugin-clerk
  • build/tooling/chore

@panteliselef panteliselef self-assigned this Oct 16, 2023
@panteliselef panteliselef requested a review from a team as a code owner October 16, 2023 21:24
@changeset-bot
Copy link

changeset-bot bot commented Oct 16, 2023

🦋 Changeset detected

Latest commit: 9a375d9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@clerk/clerk-js Patch
@clerk/types Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch
@clerk/backend Patch
@clerk/fastify Patch
gatsby-plugin-clerk Patch
@clerk/localizations Patch
@clerk/nextjs Patch
@clerk/clerk-react Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@panteliselef panteliselef changed the title Org 286 [WIP] Org 286 Oct 17, 2023
@panteliselef panteliselef changed the title [WIP] Org 286 feat(clerk-js): Use org:sys_domains:read for improved fine grain control of the UI Oct 17, 2023
@panteliselef panteliselef requested a review from anagstef October 17, 2023 16:11
},
]}
/>
showDotMenu ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

🙃 showDotMeny && <JSX/>

Copy link
Member Author

Choose a reason for hiding this comment

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

Already tried that, seems like trailingComponent does not accept null, I didn't want to mess with the signature of the component.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we update its types?

Copy link
Member Author

Choose a reason for hiding this comment

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

The following has type of false | EmotionJSX.Element | null

showDotMenu && (
                    <DomainListDotMenu
                      redirectSubPath={redirectSubPath}
                      domainId={d.id}
                    />
                  )

So i would have to do

(showDotMenu  || null ) && (
                    <DomainListDotMenu
                      redirectSubPath={redirectSubPath}
                      domainId={d.id}
                    />
                  )

Which i'm not convinced it is better

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand what you are talking about. Is this the type?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the TS error
image

@@ -35,7 +35,7 @@ export const useFetch = <T>(
requestStatus.setError();
setData(null);
});
}, []);
}, [JSON.stringify(params)]);
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Can you elaborate on this change? Is it necessary to be part of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously useFetch would only fire once, "on mount", there was a bug where the Gate component would not hide the content while not having the necessary permission because the "fetch" wouldn't fire when the parameters changed.

For context we are using useFetch to handle the async logic of isAuthorized.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to make a FAPI call every time the isAuthorized is invoked?

Copy link
Member Author

Choose a reason for hiding this comment

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

This would only invoke a call if params change, not on every render.

packages/clerk-js/src/ui/utils/test/mockHelpers.ts Outdated Show resolved Hide resolved
* @returns {string} Returns the string without leading slashes
* @param path
*/
export const trimLeadingSlash = (path: string): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a couple of tests for this util?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeap, thanks for the reminder

@panteliselef panteliselef force-pushed the ORG-286 branch 2 times, most recently from a591162 to d19c8a0 Compare October 18, 2023 13:21
packages/clerk-js/src/core/resources/Session.ts Outdated Show resolved Hide resolved
packages/clerk-js/src/core/resources/Session.ts Outdated Show resolved Hide resolved
},
]}
/>
showDotMenu ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we update its types?

@@ -35,7 +35,7 @@ export const useFetch = <T>(
requestStatus.setError();
setData(null);
});
}, []);
}, [JSON.stringify(params)]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to make a FAPI call every time the isAuthorized is invoked?

packages/clerk-js/src/ui/utils/test/mockHelpers.ts Outdated Show resolved Hide resolved
@panteliselef panteliselef changed the base branch from main to main-v4 October 25, 2023 18:53
},
]}
/>
showDotMenu ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand what you are talking about. Is this the type?

@panteliselef panteliselef added this pull request to the merge queue Oct 31, 2023
Merged via the queue into main-v4 with commit f6f67f9 Oct 31, 2023
@panteliselef panteliselef deleted the ORG-286 branch October 31, 2023 17:05
panteliselef added a commit that referenced this pull request Oct 31, 2023
github-merge-queue bot pushed a commit that referenced this pull request Oct 31, 2023
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.

4 participants