feat(payment): PAYPAL-6499 add wallet buttons#3062
feat(payment): PAYPAL-6499 add wallet buttons#3062bc-yaroslav-zhmutskyi wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: fb2e096 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Bundle Size ReportComparing against baseline from
Per-Route First Load JS
|
574a0b4 to
75cfe8a
Compare
Unlighthouse Performance Comparison — VercelComparing PR preview deployment Unlighthouse scores vs production Unlighthouse scores. Summary ScoreAggregate score across all categories as reported by Unlighthouse.
Category Scores
Core Web Vitals
|
75cfe8a to
210c887
Compare
210c887 to
ecf5807
Compare
ecf5807 to
82c8ffb
Compare
| # Disable automatic fetch instrumentation (not recommended unless using custom instrumentation) | ||
| # NEXT_OTEL_FETCH_DISABLED=1 | ||
|
|
||
| # Checkout SDK loader URL used in local development for wallet buttons. |
There was a problem hiding this comment.
Mention that the related files should be places in the core/public/v1 folder to be able use them via next server in dev mode
|
Also can you please provide some information regarding how the release environment works here? How the release flow should be managed in our case? Probably it should be covered by exp or something before merging. |
82c8ffb to
fb2e096
Compare
Our changes will be covered by experiment on backend side storefront service and will be disabled in prod for now. |
| export async function getPreferredCurrencyCode(code?: string): Promise<CurrencyCode | undefined> { | ||
| const cookieStore = await cookies(); | ||
| const currencyCode = cookieStore.get('currencyCode')?.value; | ||
| const currencyCode = cookieStore.get('currencyCode')?.value || code; |
There was a problem hiding this comment.
cookie || code means the cookie always overrides the code arg, so getCurrencyData(cart.currencyCode) can get silently overridden by a stale currency cookie. Cart currency's fixed at creation, so this can mismatch amount vs currency on the wallet button. I would say leave this helper's signature alone (~20 other call sites) and just do (await getPreferredCurrencyCode()) ?? currencyCode inline in getCurrencyData.
| const isMountedRef = useRef(false); | ||
| const initButtonProps = useStreamable(walletButtonsInitOptions); | ||
|
|
||
| useEffect(() => { | ||
| if (!isMountedRef.current && initButtonProps.length) { | ||
| isMountedRef.current = true; | ||
|
|
||
| const initWalletButtons = async () => { | ||
| await new WalletButtonsInitializer().initialize(initButtonProps); | ||
| }; | ||
|
|
||
| void initWalletButtons(); | ||
| } | ||
| }, [cartId, initButtonProps]); |
There was a problem hiding this comment.
isMountedRef makes this a one-shot init forever. Deps [cartId, initButtonProps] say "re-run on change," but the ref blocks that after the first run, so a quantity change won't update the PayPal amount. I would fix this before merge.
| isMountedRef.current = true; | ||
|
|
||
| const initWalletButtons = async () => { | ||
| await new WalletButtonsInitializer().initialize(initButtonProps); | ||
| }; | ||
|
|
||
| void initWalletButtons(); |
There was a problem hiding this comment.
isMountedRef gets set before initialize() resolves, and void initWalletButtons() has no .catch. If the SDK script fails to load, that rejection's swallowed and it never retries, wallet buttons just silently never show up. Worth a .catch that resets the ref.
| cartId: string; | ||
| }) => { | ||
| const isMountedRef = useRef(false); | ||
| const initButtonProps = useStreamable(walletButtonsInitOptions); |
There was a problem hiding this comment.
This runs outside the <Stream fallback={null}> boundary below, so a rejected promise throws into render instead of degrading gracefully like the fallback implies. Might be worth moving the unwrap inside Stream's render prop.
| {(buttonOptions) => ( | ||
| <div style={{ display: 'flex', alignItems: 'end', flexDirection: 'column' }}> | ||
| {buttonOptions.map((button) => | ||
| button.containerId ? <div id={button.containerId} key={button.containerId} /> : null, |
There was a problem hiding this comment.
Nit: containerId is required and always set, so : null here is dead. Could just be buttonOptions.map((button) => <div id={button.containerId} key={button.containerId} />).
| type PaymentWalletsVariables = VariablesOf<typeof PaymentWalletsQuery>; | ||
|
|
||
| export const getPaymentWallets = async (variables: PaymentWalletsVariables) => { | ||
| const customerAccessToken = await getSessionCustomerAccessToken(); |
There was a problem hiding this comment.
getSessionCustomerAccessToken isn't cache()'d, so with N wallets it decodes the session N+2 times per render. Might be worth wrapping it in cache().
| * - robots.txt (robots route) | ||
| */ | ||
| '/((?!api|admin|_next/static|_next/image|_vercel|favicon.ico|xmlsitemap.php|sitemap.xml|robots.txt).*)', | ||
| '/((?!api|admin|_next/static|_next/image|v1|_vercel|favicon.ico|xmlsitemap.php|sitemap.xml|robots.txt).*)', |
There was a problem hiding this comment.
This v1 isn't segment-anchored, so it'll match any slug starting with "v1" (e.g. /v1-turbo-kit), not just the loader path. Might want v1/ instead.
| GiftCertificateCodeFormState, | ||
| } from '@/vibes/soul/sections/cart/gift-certificate-code-form'; | ||
| import { StickySidebarLayout } from '@/vibes/soul/sections/sticky-sidebar-layout'; | ||
| import { ClientWalletButtons } from 'components/wallet-buttons'; |
There was a problem hiding this comment.
Two things: this import should be ~/components/wallet-buttons like the rest of the file. Also, vibes/soul's supposed to stay presentation-only, this pulls in app-level wallet logic plus a cartId prop. Could pass ClientWalletButtons in as a ReactNode slot instead (see product-detail's additionalActions).
| const checkoutButtonModule = await window.checkoutKitLoader.load('wallet-button'); | ||
|
|
||
| return checkoutButtonModule.createWalletButtonInitializer({ | ||
| graphQLEndpoint: '/graphql', |
There was a problem hiding this comment.
We probably should react to the trailing slash environment variable and append it if it's enabled.
| # Checkout SDK loader URL used in local development for wallet buttons. | ||
| # In non-development environments the production CDN loader is always used. | ||
| # Related files can be placed in core/public/v1 so they are served by the Next server in dev mode | ||
| NEXT_PUBLIC_CHECKOUT_SDK_DEV_URL=http://localhost:3000/v1/loader.js |
There was a problem hiding this comment.
Can we avoid having to drop the file in the local Catalyst folder? My thoughts here is that since you are trying to make changes in both repos, you can have two separate running dev processes:
- In Catalyst,
http://localhost:3000/... - In
checkout-sdk-js,http://localhost:3001/v1/loader.js
This way we can get rid of the proxy.ts change below, you can make changes in both repos without having to rebuild, we don't need to drop a file into core/public/.
What/Why?
Add wallet button integration for cart page
Testing
Manual
video_paypal.mp4
Migration
No need