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,types,nextjs,remix): Terse path parameters #1957

Merged
merged 31 commits into from
Nov 23, 2023

Conversation

octoper
Copy link
Member

@octoper octoper commented Oct 30, 2023

Description

This PR introduces some breaking changes

  • By default, all components that use routing will have their routing prop assigned with 'path' if the path prop is filled. Additionally, the types will be stricter, and if the path prop is filled and the routing prop is filled with a value other than 'path', TypeScript will throw an error.
  • The <UserButton /> component will have the userProfileMode prop assigned with 'navigation' by default if the userProfileUrl prop is filled. Similarly, the types will be stricter, and if the userProfileUrl prop is filled and the userProfileMode prop is filled with a value other than 'navigation', TypeScript will throw an error.
  • The <OrganizationSwitcher /> component will have the organizationProfileMode and createOrganizationMode props assigned with 'navigation' by default if the organizationProfileUrl and createOrganizationUrl props are filled accordingly. Additionally, the types will be stricter, and if the organizationProfileUrl or createOrganizationUrl prop is filled and the organizationProfileMode or createOrganizationMode prop is filled with a value other than 'navigation', TypeScript will throw an error.

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

@changeset-bot
Copy link

changeset-bot bot commented Oct 30, 2023

🦋 Changeset detected

Latest commit: 25f9cc8

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

This PR includes changesets to release 11 packages
Name Type
@clerk/clerk-js Major
@clerk/nextjs Minor
@clerk/clerk-react Minor
@clerk/remix Minor
@clerk/types Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch
gatsby-plugin-clerk Patch
@clerk/backend Patch
@clerk/fastify 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

@octoper octoper force-pushed the george/js-31-terse-paths-params branch 2 times, most recently from 85f1cac to 8daae51 Compare October 30, 2023 10:51
@octoper octoper self-assigned this Oct 30, 2023
@octoper octoper force-pushed the george/js-31-terse-paths-params branch from f31ab32 to e0bc858 Compare October 31, 2023 07:15
@octoper octoper marked this pull request as ready for review October 31, 2023 07:37
@octoper octoper force-pushed the george/js-31-terse-paths-params branch 2 times, most recently from 38f0c70 to 2f488d8 Compare November 1, 2023 11:10
@octoper octoper requested a review from a team November 1, 2023 11:11
@octoper octoper force-pushed the george/js-31-terse-paths-params branch 3 times, most recently from 4d19069 to 69b523d Compare November 2, 2023 21:24
@dimkl dimkl force-pushed the george/js-31-terse-paths-params branch from a4b9d16 to 23f552d Compare November 6, 2023 11:31
@octoper octoper force-pushed the george/js-31-terse-paths-params branch 3 times, most recently from 11e7ea1 to 00477d4 Compare November 7, 2023 18:32
@octoper octoper requested a review from dimkl November 7, 2023 18:32
Copy link
Contributor

@desiprisg desiprisg left a comment

Choose a reason for hiding this comment

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

Good work! We should add some type tests to make sure this doesn't break in the future since these are some complex types. Check this file for referennce: https://github.com/clerkinc/javascript/blob/3ceb2a734a43f134956164377399fec46e01e0a1/packages/react/src/contexts/__tests__/ClerkProvider.test.tsx

@octoper octoper force-pushed the george/js-31-terse-paths-params branch from c90d57e to d8854b0 Compare November 8, 2023 18:20
@dimkl dimkl requested a review from desiprisg November 9, 2023 12:32
@octoper octoper force-pushed the george/js-31-terse-paths-params branch from a1f1a08 to 542012a Compare November 9, 2023 13:14
Copy link
Member

@nikosdouvlis nikosdouvlis left a comment

Choose a reason for hiding this comment

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

@octoper You will need to apply the changes to the ui-retheme dir. I will share more details within the day

Copy link
Member

@nikosdouvlis nikosdouvlis left a comment

Choose a reason for hiding this comment

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

I noticed that you updated the next-app-router and the react-vite apps and added the /hash pages - is there any reason you did not update the rest?

In general, every app in the template dir must have the same interface so we can use the apps interchangeably throughout the tests.

If you want to run a small navigation-focused suite without affecting any templates, you can use an existing applicationConfig and the addFile method to programmatically create an app before the tests. The scope of the app will be just that specific test

integration/tests/navigation.test.ts Outdated Show resolved Hide resolved
integration/tests/navigation.test.ts Outdated Show resolved Hide resolved
integration/tests/navigation.test.ts Outdated Show resolved Hide resolved
@@ -112,3 +112,17 @@ export const buildAuthQueryString = (data: BuildAuthQueryStringArgs): string | n
}
return Object.keys(query).length === 0 ? null : qs.stringify(query);
};

export const normalizeRoutingOptions = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

🙃 This file is not just about authPropHelpers anymore. It is a collection of prop/options parsers and normalizers. So, at some point, we need to rename it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted!

@nikosdouvlis nikosdouvlis merged commit 7f6a64f into main Nov 23, 2023
7 checks passed
@nikosdouvlis nikosdouvlis deleted the george/js-31-terse-paths-params branch November 23, 2023 09:11
desiprisg pushed a commit that referenced this pull request Nov 23, 2023
* feat(clerk-js,types,nextjs,remix): Terse paths parameters

* chore(repo): Warning instead of erroring for no unused variables rule

* chore(repo): Remove changes from NextJS playground

* feat(clerk-js,clerk-react,types): Terse params for OrganizationSwitcher and UserButton

* fix(clerk-js): Use only userProfileUrl prop to check what userProfileMode should be used

* test(clerk-js): Added tests for normalizeRoutingOptions utility function

* chore(repo): Added Changeset

* feat(clerk-js): Omit routing prop from Modal components

* chore(types): Fix typo

* fix(clerk-js): Check all falsy values instead of just undefined

* test(clerk-js): Fix tests for normalizeRoutingOptions

* feat(types): Added WithoutRouting type helper

* fix(remix): Revert SignIn and SIgnUp components

* chore(clerk-js): Remove uneeded import

* fix(remix): Revert changes

* fix(remix,nextjs,types): Components with routing

* test(clerk-react): Added types tests for SignIn component

* test(clerk-react): Added types tests for SignUp component

* test(clerk-react): Added types tests for UserButton component

* test(clerk-react): Added types tests for OrganizationSwitcher component

* test(clerk-react): Added type tests for UserProfile and OrganizationProfile components

* feat(clerk-js): Apply changes to ui.retheme

* refactor(clerk-js): Make normalizeRoutingOptions more verbose

* chore(clerk-js): Remove unedeed type

* refactor(clerk-js,types): Add types for modal components

* refactor(nextjs,remix): Refactor SignIn/SignUp components for Next and Remix

* tests(repo): Add integration tests

* chore(repo): Fix formatting

* test(repo): Update navigation integration tests

* test(repo): Update navigation integration tests

* test(repo): Add more test cases for navigation integration tests
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.

7 participants