Skip to content

fix(clerk-js): Respect props passed to buildSignInUrl and buildSignUpUrl - #3361

Merged
nikosdouvlis merged 2 commits into
mainfrom
nikos/sdk-1720-forceredirecturl-is-not-working-when-using-signinbutton
May 13, 2024
Merged

fix(clerk-js): Respect props passed to buildSignInUrl and buildSignUpUrl#3361
nikosdouvlis merged 2 commits into
mainfrom
nikos/sdk-1720-forceredirecturl-is-not-working-when-using-signinbutton

Conversation

@nikosdouvlis

@nikosdouvlis nikosdouvlis commented May 10, 2024

Copy link
Copy Markdown
Member

Description

This PR updates the buildSignInUrl and buildSignUpUrl methods to correctly propagate the redirect props via URL search params. This allows the SignInButton and SignUpButton components to be configurable via their props as well.

Additionally, this PR:

  • simplifies the RedirectUrl class by dropping the concern of appending params to the URL. This is now handled by the low-level buildUrl util
  • it fixes an issues when serializing initial values in the buildSignInUrl and buildSignUpUrl helpers. Search params need to be snake_cased in order to be respected
  • buildUrl now accepts a new hashSearchParams prop and it snake_cases all search params by default

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:

@changeset-bot

changeset-bot Bot commented May 10, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: a4ef2c6

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

This PR includes changesets to release 7 packages
Name Type
@clerk/clerk-js Patch
@clerk/clerk-react Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch
@clerk/elements Patch
@clerk/nextjs Patch
@clerk/remix 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

@nikosdouvlis
nikosdouvlis marked this pull request as draft May 10, 2024 12:46
return sameOrigin ? stripOrigin(url) : `${url}`;
};

export const appendAsQueryParams = (

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Killed this as we no longer need it

}

// Keep just the pathname and the search
// Merge search params from the hashSearchParams object

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

All search params appended by clerk-js are snake_cased. Moved the logic here to make sure that the caller has to remember/think about this detail

// Keep just the pathname and the search
// Merge search params from the hashSearchParams object
if (hashSearchParams) {
const paramsArr = Array.isArray(hashSearchParams) ? hashSearchParams : [hashSearchParams];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Previously, the util supported passing hash params as a string only but the best way here is to simply pass a URLSearchParams object instead. I added the new functionality but kept the old one, will refactor all usages to the new format in a new PR

);
}

#flattenAll() {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I decided to stick with a more verbose approach here instead of a a more complex loop of some sort, let me know if you this is is too much

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like it

@nikosdouvlis
nikosdouvlis requested a review from desiprisg May 10, 2024 13:15
@nikosdouvlis
nikosdouvlis force-pushed the nikos/sdk-1720-forceredirecturl-is-not-working-when-using-signinbutton branch from 7697adb to e607864 Compare May 10, 2024 14:07
@nikosdouvlis
nikosdouvlis marked this pull request as ready for review May 10, 2024 14:07
@nikosdouvlis
nikosdouvlis force-pushed the nikos/sdk-1720-forceredirecturl-is-not-working-when-using-signinbutton branch 2 times, most recently from 03ee073 to 7cd88d6 Compare May 10, 2024 14:23

@brkalow brkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👏

@nikosdouvlis
nikosdouvlis force-pushed the nikos/sdk-1720-forceredirecturl-is-not-working-when-using-signinbutton branch 2 times, most recently from a73f777 to b73c18e Compare May 10, 2024 22:16
@LekoArts LekoArts changed the title Respect props passed to buildSignInUrl and buildSignUpUrl fix(clerk-js): Respect props passed to buildSignInUrl and buildSignUpUrl May 13, 2024
Comment thread packages/clerk-js/src/core/clerk.ts Outdated
Comment thread packages/clerk-js/src/core/clerk.ts Outdated
Comment thread packages/clerk-js/src/ui/router/RouteContext.tsx
Comment thread packages/clerk-js/src/utils/__tests__/url.test.ts
Comment thread packages/react/src/components/__tests__/SignInButton.test.tsx Outdated
Comment thread packages/react/src/components/__tests__/SignUpButton.test.tsx
Comment thread .changeset/wet-peaches-grow.md Outdated
@nikosdouvlis
nikosdouvlis force-pushed the nikos/sdk-1720-forceredirecturl-is-not-working-when-using-signinbutton branch from b73c18e to 99372db Compare May 13, 2024 08:50
@nikosdouvlis
nikosdouvlis requested a review from LekoArts May 13, 2024 08:51
@desiprisg

Copy link
Copy Markdown
Contributor

!preview

@clerk-cookie

clerk-cookie commented May 13, 2024

Copy link
Copy Markdown
Collaborator

Hey @desiprisg, your preview is available.

Status Preview Updated (UTC)
🍪 Deployed Visit preview May 13, 2024 09:09 AM

@desiprisg

Copy link
Copy Markdown
Contributor

!snapshot

@clerk-cookie

Copy link
Copy Markdown
Collaborator

Hey @desiprisg - the snapshot version command generated the following package versions:

Package Version
@clerk/chrome-extension 1.0.8-snapshot.v99372db
@clerk/clerk-js 5.2.4-snapshot.v99372db
@clerk/elements 0.3.4-snapshot.v99372db
@clerk/clerk-expo 1.0.8-snapshot.v99372db
gatsby-plugin-clerk 5.0.0-beta.45
@clerk/nextjs 5.0.8-snapshot.v99372db
@clerk/clerk-react 5.0.5-snapshot.v99372db
@clerk/remix 4.0.6-snapshot.v99372db

Tip: Use the snippet copy button below to quickly install the required packages.
@clerk/chrome-extension

npm i @clerk/chrome-extension@1.0.8-snapshot.v99372db --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@5.2.4-snapshot.v99372db --save-exact

@clerk/elements

npm i @clerk/elements@0.3.4-snapshot.v99372db --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@1.0.8-snapshot.v99372db --save-exact

gatsby-plugin-clerk

npm i gatsby-plugin-clerk@5.0.0-beta.45 --save-exact

@clerk/nextjs

npm i @clerk/nextjs@5.0.8-snapshot.v99372db --save-exact

@clerk/clerk-react

npm i @clerk/clerk-react@5.0.5-snapshot.v99372db --save-exact

@clerk/remix

npm i @clerk/remix@4.0.6-snapshot.v99372db --save-exact

@nikosdouvlis
nikosdouvlis force-pushed the nikos/sdk-1720-forceredirecturl-is-not-working-when-using-signinbutton branch from 99372db to a4ef2c6 Compare May 13, 2024 09:18

@desiprisg desiprisg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM but let's state that this won't work with core-2 app and core-1 account portal.

@nikosdouvlis
nikosdouvlis merged commit bcbb2c9 into main May 13, 2024
@nikosdouvlis
nikosdouvlis deleted the nikos/sdk-1720-forceredirecturl-is-not-working-when-using-signinbutton branch May 13, 2024 13:12
@clerk-cookie clerk-cookie mentioned this pull request May 13, 2024
#buildUrl = (
key: 'signInUrl' | 'signUpUrl',
options: RedirectOptions,
_initValues?: Record<string, string>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🙃 The _ prefix in parameters is used to mark unused parameters of a function. Are we sure about this naming?

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.

6 participants