fuentesloic / nuxt-stripe

MIT License
90 stars 8 forks source link

feat(breaking): various improvements to both server and client side + types #19

Closed sandros94 closed 1 year ago

sandros94 commented 1 year ago

Fixes #18 Fixes #20 Fixes #23

What does this PR do?

This PR fixes a number of things:

What are the breaking changes?

changed env naming

Following the naming conventions for Nuxt the env variables should be followed:

-STRIPE_PUBLISHABLE_KEY="pk_live_..."
-STRIPE_API_KEY="sk_live_..."
+NUXT_PUBLIC_STRIPE_PUBLISHABLE_KEY = 'pk_live_...'
+NUXT_STRIPE_API_KEY = 'sk_live_...'

apiVersion config

To make server side configurations available for nuxt-stripe, the apiVersion placement has been changed and the following must be taken in consideration:

export default defineNuxtConfig({
  modules: [
    '@unlok-co/nuxt-stripe'
  ],
  stripe: {
    // Server
    apiKey: 'sk_test_123', // required
-   apiVersion: '2022-11-15', // optional, default is '2022-11-15'
+   serverConfig: {
+    apiVersion: '2022-11-15', // optional, default is '2022-11-15'
+   },
    // Client
    publishableKey: 'pk_test_123', // required
+   clientConfig: {
+    apiVersion: '2022-11-15', // optional, default is '2022-11-15'
+   },
  }
})

Original Post

Still not fully sure if this is the correct way to do this, but basically this small change already enables us to provide and edit stripe keys at runtime.

@danielroe Sorry to disturb, could you take a quick look if I'm doing things decently?

danielroe commented 1 year ago

Yes, @Sandros94, this should be good 👍 But you could also perhaps initialise them to empty strings in the default options.

sandros94 commented 1 year ago

Yes, @Sandros94, this should be good 👍 But you could also perhaps initialise them to empty strings in the default options.

Sorry to be redundant, asking to make sure I'm 100% of understanding, do you mean in the module.ts right?

https://github.com/fuentesloic/nuxt-stripe/blob/23c74938a3c10173f07c451dba3fbb20fb493931/src/module.ts#L39-L43

Becoming something like is enough?:

defaults: { 
     publishableKey: '' as string, 
     apiKey: '' as string, 
     apiVersion: '2022-11-15' 
 }, 
sandros94 commented 1 year ago

I decided to revert the commit for now, this means that stripe-js is loaded twice (first from the plugin then from the component) and it will be loaded globally breaking the lazy loading and a number of page optimizations (that impact negatively on Lighthouse scores).

sandros94 commented 1 year ago

I've found the issue, and I didn't think about it at first.

Following the stripe-js doc:

This function returns a Promise that resolves with a newly created Stripe object once Stripe.js has loaded. [...] If you call loadStripe in a server environment it will resolve to null.

import {loadStripe} from '@stripe/stripe-js';

const stripe = await loadStripe('pk_test_TYooMQauvdEDq54NiTphI7jx');

Since loadStripe function already checks for server/client side loading, we don't need to add any logics for that.

But, loadStripe returns a Promise, so we need to await for it. Since the async nature of it, this lets us load the package only on the pages defined by the devs that want it.

<template>
  <main>
    <h1>Stripe client</h1>
    <pre>
      {{ stripeClient ? stripeClient : 'Loading...' }}
    </pre>
  </main>
</template>

<script setup lang="ts">
const stripeClient = await useClientStripe()
</script>
sandros94 commented 1 year ago

Not to mention that this new useClientStripe lets us expose all the stripe-js types for better ts support.

IMO this PR is currently complete. @fuentesloic

sandros94 commented 1 year ago

I still not fully understand why #22 was created, but it pointed out an important, missing, feature in the current module: the ability to config and change stripeConfig for useServerStripe. I've made sure to include and export the related types and update the vitests.

This introduce a breaking change, because apiVersion is now configured inside the new serverConfig.

I still need to understand if stripe-js does have such options or are all defined at load.

sandros94 commented 1 year ago

I've checked the stripe-js docs and I've found the options mentioned, they aren't many but it was an easy addition so I decided to add them regardless.

fuentesloic commented 1 year ago

@Sandros94 I'm going to merge your PR and check with #22 the following path to keep in order to limit futur breaking change. I think all the work propose make sense so it will be release once every breaking change has been merged. Thanks again for your contribution 💪