-
Notifications
You must be signed in to change notification settings - Fork 280
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
fix(clerk-js,clerk-react,types): Optimize Coinbase SDK loading #4786
base: main
Are you sure you want to change the base?
fix(clerk-js,clerk-react,types): Optimize Coinbase SDK loading #4786
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: a83056f The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
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 |
7217c73
to
db3ef38
Compare
db3ef38
to
c0492ab
Compare
c0492ab
to
57f613d
Compare
!snapshot |
Hey @nikospapcom - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/astro@2.1.3-snapshot.v20241217094601 --save-exact
npm i @clerk/backend@1.21.3-snapshot.v20241217094601 --save-exact
npm i @clerk/chrome-extension@2.1.4-snapshot.v20241217094601 --save-exact
npm i @clerk/clerk-js@5.43.1-snapshot.v20241217094601 --save-exact
npm i @clerk/elements@0.22.3-snapshot.v20241217094601 --save-exact
npm i @clerk/clerk-expo@2.6.3-snapshot.v20241217094601 --save-exact
npm i @clerk/expo-passkeys@0.1.3-snapshot.v20241217094601 --save-exact
npm i @clerk/express@1.3.30-snapshot.v20241217094601 --save-exact
npm i @clerk/fastify@2.1.3-snapshot.v20241217094601 --save-exact
npm i @clerk/localizations@3.9.4-snapshot.v20241217094601 --save-exact
npm i @clerk/nextjs@6.9.4-snapshot.v20241217094601 --save-exact
npm i @clerk/nuxt@0.1.4-snapshot.v20241217094601 --save-exact
npm i @clerk/clerk-react@5.20.3-snapshot.v20241217094601 --save-exact
npm i @clerk/react-router@0.1.3-snapshot.v20241217094601 --save-exact
npm i @clerk/remix@4.4.4-snapshot.v20241217094601 --save-exact
npm i @clerk/clerk-sdk-node@5.1.3-snapshot.v20241217094601 --save-exact
npm i @clerk/shared@2.20.3-snapshot.v20241217094601 --save-exact
npm i @clerk/tanstack-start@0.8.3-snapshot.v20241217094601 --save-exact
npm i @clerk/testing@1.4.3-snapshot.v20241217094601 --save-exact
npm i @clerk/themes@2.2.3-snapshot.v20241217094601 --save-exact
npm i @clerk/types@4.39.5-snapshot.v20241217094601 --save-exact
npm i @clerk/ui@0.3.4-snapshot.v20241217094601 --save-exact
npm i @clerk/vue@0.1.4-snapshot.v20241217094601 --save-exact |
a276304
to
315564b
Compare
packages/clerk-js/src/core/clerk.ts
Outdated
get isCoinbaseWalletEnabled(): boolean { | ||
const attributes = this.environment?.userSettings.attributes; | ||
if (!attributes) { | ||
return false; | ||
} | ||
|
||
const findWeb3Attributes = Object.entries(attributes) | ||
.filter(([name, attr]) => attr.used_for_first_factor && name.startsWith('web3')) | ||
.map(([, desc]) => desc.first_factors) | ||
.flat() as any as Web3Strategy[]; | ||
|
||
return findWeb3Attributes.includes('web3_coinbase_wallet_signature'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓Should we make this private ? Since we don't want any api changes atm
packages/clerk-js/src/core/clerk.ts
Outdated
@@ -1654,6 +1672,16 @@ export class Clerk implements ClerkInterface { | |||
this.environment = environment; | |||
} | |||
|
|||
public prefetchDependencies = async (): Promise<void> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can be private, and don't need to call it from IsomorphicClerk
.
packages/clerk-js/src/core/clerk.ts
Outdated
@@ -180,6 +183,7 @@ export class Clerk implements ClerkInterface { | |||
protected internal_last_error: ClerkAPIError | null = null; | |||
// converted to protected environment to support `updateEnvironment` type assertion | |||
protected environment?: EnvironmentResource | null; | |||
private __promise = createDeferredPromise(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private __promise = createDeferredPromise(); | |
#coinbaseSDKResolvers = createDeferredPromise(); |
packages/clerk-js/src/core/clerk.ts
Outdated
@@ -1654,6 +1672,16 @@ export class Clerk implements ClerkInterface { | |||
this.environment = environment; | |||
} | |||
|
|||
public prefetchDependencies = async (): Promise<void> => { | |||
if (this.isCoinbaseWalletEnabled) { | |||
window.__clerk_coinbase_sdk = this.__promise.promise as Promise<typeof CoinbaseWalletSDK>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this need to move inside the constructor.
Also
window.__clerk_coinbase_sdk = this.__promise.promise as Promise<typeof CoinbaseWalletSDK>; | |
window.__clerk_coinbase_sdk = this.#coinbaseSDKResolvers.promise as Promise<typeof CoinbaseWalletSDK>; |
packages/clerk-js/src/core/clerk.ts
Outdated
await import('@coinbase/wallet-sdk').then(mod => { | ||
this.__promise.resolve(mod.CoinbaseWalletSDK); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await import('@coinbase/wallet-sdk').then(mod => { | |
this.__promise.resolve(mod.CoinbaseWalletSDK); | |
}); | |
await import('@coinbase/wallet-sdk').then(mod => { | |
this.#coinbaseSDKResolvers.resolve(mod.CoinbaseWalletSDK); | |
}); |
packages/clerk-js/src/utils/web3.ts
Outdated
@@ -71,7 +71,7 @@ export async function generateSignatureWithOKXWallet(params: GenerateSignaturePa | |||
|
|||
async function getEthereumProvider(provider: Web3Provider) { | |||
if (provider === 'coinbase_wallet') { | |||
const CoinbaseWalletSDK = await import('@coinbase/wallet-sdk').then(mod => mod.CoinbaseWalletSDK); | |||
const CoinbaseWalletSDK = await window.__clerk_coinbase_sdk; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const CoinbaseWalletSDK = await window.__clerk_coinbase_sdk; | |
const CoinbaseWalletSDK = await window.__clerk_coinbase_sdk.catch(()=>null) | |
// example error | |
if(!CoinbaseWalletSDK) { throw "Coinbase SDK failed to load" } |
|
||
prefetchDependencies = async (): Promise<void> => { | ||
const callback = () => this.clerkjs?.prefetchDependencies(); | ||
if (this.clerkjs && this.#loaded) { | ||
return callback() as Promise<void>; | ||
} else { | ||
this.premountMethodCalls.set('prefetchDependencies', callback); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹🧹 we don't really need these with this approach
packages/clerk-js/src/core/clerk.ts
Outdated
@@ -1654,6 +1672,16 @@ export class Clerk implements ClerkInterface { | |||
this.environment = environment; | |||
} | |||
|
|||
public prefetchDependencies = async (): Promise<void> => { | |||
if (this.isCoinbaseWalletEnabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an optimization we should add the follwing
if(signedIn && singleMode) then don't load the sdk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@panteliselef we need to load when user in signedIn
because users can connect the wallet through <UserProfile />
Instead of lazy loading the Coinbase SDK in getEthereumProvider(), move loading when the SignIn/SignUp components mount. This fix avoid the Safari to block the Coinbase popup
c5315fd
to
a83056f
Compare
Introduce new method
prefetchDependencies
to load Coinbase SDK instead of loading the SDK when click theCoinbase
button in<SignIn/>
/<SignUp />
. This change avoid the Safari to block the Coinbase popupDescription
Checklist
pnpm test
runs as expected.pnpm build
runs as expected.Type of change