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.

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

Jump to bottom

feat(clerk-js): Change the default behaviour of afterSignOutUrl, afterSignIn and afterSignUp #2020

Merged
merged 22 commits into from
Dec 6, 2023
Merged

feat(clerk-js): Change the default behaviour of afterSignOutUrl, afterSignIn and afterSignUp #2020

merged 22 commits into from
Dec 6, 2023

Conversation

octoper
Copy link
Member

@octoper octoper commented Nov 2, 2023 •
edited
Loading

Description

This PR changes the default behaviour of the following

  • Change the default behaviour of after afterSignOutUrl prop in <UserButton /> to be redirected to / instead of the Account Portal defined url.
  • Change the default behaviour of after afterSignIn and afterSignUp props to be redirected to / instead of the Account Portal defined url.
  • Use the default options for the Clerk singleton as right now they get overwritten from undefined values.
  • Added a utility function to @clerk/shared package, that receives an object and returns it back without the properties undefined value.

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/backend
  • @clerk/chrome-extension
  • @clerk/clerk-js
  • @clerk/clerk-expo
  • @clerk/fastify
  • gatsby-plugin-clerk
  • @clerk/localizations
  • @clerk/nextjs
  • @clerk/clerk-react
  • @clerk/remix
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/themes
  • @clerk/types
  • build/tooling/chore

@octoper octoper requested a review from a team as a code owner November 2, 2023 21:08
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 2, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 59ab8ae

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/shared Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch
@clerk/backend Patch
@clerk/fastify Patch
@clerk/nextjs Patch
@clerk/clerk-react Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch
gatsby-plugin-clerk 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 vaggelis/sdk-831-change-the-default-behavior-of-aftersignouturl-of-userbutton branch from 2e81ce1 to 91fe24d Compare November 2, 2023 21:08
@octoper octoper changed the title feat(clerk-js): Change the default behavior of afterSignOutUrl, afterSignIn and afterSignUp feat(clerk-js): Change the default behaviour of afterSignOutUrl, afterSignIn and afterSignUp Nov 2, 2023
anagstef
anagstef reviewed Nov 3, 2023
View reviewed changes
packages/clerk-js/src/core/clerk.ts Outdated
Comment on lines 143 to 134
afterSignInUrl: '/',
afterSignUpUrl: '/',
Copy link
Member

Choose a reason for hiding this comment

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

There are more places in our codebase where we use displayConfig.afterSignInUrl and displayConfig.afterSignUpUrl. Are those also considered?

Copy link
Member Author

@octoper octoper Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

After the changes I made the default values will be directly taken from the defaultOptions so the displayConfig.afterSignInUrl or displayConfig.afterSignUpUrl are never used.

@dimkl dimkl force-pushed the vaggelis/sdk-831-change-the-default-behavior-of-aftersignouturl-of-userbutton branch from 7aecc46 to eae95f2 Compare November 6, 2023 11:29
@octoper octoper force-pushed the vaggelis/sdk-831-change-the-default-behavior-of-aftersignouturl-of-userbutton branch 2 times, most recently from 35db2db to 0ffd8bf Compare November 7, 2023 11:02
@octoper octoper force-pushed the vaggelis/sdk-831-change-the-default-behavior-of-aftersignouturl-of-userbutton branch 2 times, most recently from 42e46a1 to 320b770 Compare November 8, 2023 11:05
nikosdouvlis
nikosdouvlis requested changes Nov 9, 2023
View reviewed changes
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

@octoper octoper force-pushed the vaggelis/sdk-831-change-the-default-behavior-of-aftersignouturl-of-userbutton branch 2 times, most recently from 74cc3f8 to 9997c00 Compare November 10, 2023 10:02
@octoper octoper force-pushed the vaggelis/sdk-831-change-the-default-behavior-of-aftersignouturl-of-userbutton branch 3 times, most recently from 5c30f34 to 15b3a32 Compare November 15, 2023 08:47
anagstef
anagstef reviewed Nov 15, 2023
View reviewed changes
.changeset/spotty-boxes-do.md Outdated Show resolved Hide resolved
packages/clerk-js/src/core/clerk.redirects.test.ts Outdated Show resolved Hide resolved
@octoper octoper force-pushed the vaggelis/sdk-831-change-the-default-behavior-of-aftersignouturl-of-userbutton branch from af58073 to 021108b Compare November 15, 2023 14:46
nikosdouvlis
nikosdouvlis requested changes Nov 16, 2023
View reviewed changes
packages/clerk-js/src/core/clerk.ts Outdated
Comment on lines 133 to 134
afterSignInUrl: '/',
afterSignUpUrl: '/',
Copy link
Member

Choose a reason for hiding this comment

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

I need to take another look, but my instinct tells me that this is not the direction we want to go as this will cause these to have actual values so, by default, the URLs will include the query params.

What we want to do instead, is to change the behavior of Clerk IF these are undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Made the changes to only fallback to / and the values will not be included to the query params.

@octoper octoper force-pushed the vaggelis/sdk-831-change-the-default-behavior-of-aftersignouturl-of-userbutton branch from bf04a09 to 2c10bf3 Compare December 4, 2023 15:34
octoper added 22 commits December 6, 2023 11:10
@octoper
feat(clerk-js): Change the default path of afterSignOutUrl
5801a06
@octoper
feat(clerk-js,shared): Use default options if nothing passed when loa…
ac94a63 
…ding Clerk client
@octoper
chore(repo): Update NextJS playground
bb62c3c
@octoper
chore(clerk-js): Fix typo
3255cf4
@octoper
chore(repo): Added Changeset
b7775b3
@octoper
chore(shared): Fixed types for getNonUndefinedValues and added tests
0d4204e
@octoper
fix(remix): Utility function getEnvVariable return undefined instead …
6e4ef19 
…of empty string
@octoper
fix(clerk-js): Give empty object when options are undefined
c856132
@octoper
test(shared): Fixed test for getNonUndefinedValues utility function
936fdf2
@octoper
fix(clerk-js): Apply changes to ui.retheme
060401b
@octoper
fix(clerk-js): Add missing import
016da40
@octoper
fix(clerk-js): Add missing import
6c23963
@octoper
test(clerk-js): Update Clerk redirect tests
2260eb9
@octoper
chore(repo): Update Changeset
c58c6f0
@octoper
chore(remix): Update return type comment
e34efdd
@octoper
fix(clerk-js): Use the defaults for afterSignInUrl and afterSignUpUrl…
09ae7be 
… as fallbacks
@octoper
chore(remix): Update return type of getEnvVariable utility function
58aba6a
@octoper
test(clerk-js): Update test for SignInStart component
4fb3201
@octoper
test(clerk-js): Update test for SignInStart component in ui.retheme
b47a5c0
@octoper
feat(clerk-js): Removed display config values
e8ebc75
@octoper
chore(repo): Apply Prettier formating
74b0d29
@octoper
chore(shared): Remove unused import from tests
59ab8ae
@octoper octoper force-pushed the vaggelis/sdk-831-change-the-default-behavior-of-aftersignouturl-of-userbutton branch from 2c10bf3 to 59ab8ae Compare December 6, 2023 09:10
@octoper octoper added this pull request to the merge queue Dec 6, 2023
Merged via the queue into main with commit d30ea1f Dec 6, 2023
9 checks passed
@octoper octoper deleted the vaggelis/sdk-831-change-the-default-behavior-of-aftersignouturl-of-userbutton branch December 6, 2023 09:53
@clerk-cookie clerk-cookie mentioned this pull request Dec 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anagstef anagstef anagstef approved these changes

@panteliselef panteliselef panteliselef approved these changes

@dimkl dimkl dimkl approved these changes

@nikosdouvlis nikosdouvlis nikosdouvlis approved these changes

@SokratisVidros SokratisVidros SokratisVidros approved these changes

@LekoArts LekoArts LekoArts left review comments

Assignees

@octoper octoper

Labels
clerk-js playground remix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@octoper @dimkl @SokratisVidros @nikosdouvlis @anagstef @LekoArts @panteliselef @clerk-cookie