Add support for RedisAPL & fix Docker setup #65
Conversation
… failure urls support, support for app base path
|
João Ferreira seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
next.config.mjs
Outdated
| @@ -15,11 +15,13 @@ import { withSentryConfig } from "@sentry/nextjs"; | |||
| /** @type {import('next').NextConfig} */ | |||
| const config = { | |||
| reactStrictMode: true, | |||
| output: "standalone", | |||
There was a problem hiding this comment.
question: is that needed for the dockerfile?
| @@ -43,7 +44,7 @@ export const trpcClient = createTRPCNext<AppRouter>({ | |||
| }, | |||
| }), | |||
| httpBatchLink({ | |||
| url: "/api/trpc", | |||
| url: `${env.NEXT_PUBLIC_BASE_PATH}/api/trpc`, | |||
There was a problem hiding this comment.
question: Won't this break if env.NEXT_PUBLIC_BASE_PATH is not provided? It can be undefined AFAIK
There was a problem hiding this comment.
Yes, is true. The app should not start if those variables are not set.
|
|
||
| const server = new ApolloServer({ typeDefs, resolvers }); | ||
|
|
||
| export default startServerAndCreateNextHandler(server, { |
There was a problem hiding this comment.
question: Is this required for Apollo to work?
src/pages/api/graphql.ts
Outdated
|
|
||
| return { | ||
| token, | ||
| channel: "default-channel", |
There was a problem hiding this comment.
question/suggestion: I am not sure if this should be hardcoded. Where can we get it from?
There was a problem hiding this comment.
The channel can be removed
src/pages/api/health.ts
Outdated
| tokenTargetUrl: `${apiBaseURL}${env.NEXT_PUBLIC_BASE_PATH}/api/register`, | ||
| appUrl: `${iframeBaseUrl}${env.NEXT_PUBLIC_BASE_PATH}`, | ||
| permissions: ["HANDLE_PAYMENTS"], | ||
| version: packageJson.version, | ||
| requiredSaleorVersion: ">=3.15.0", | ||
| homepageUrl: "https://docs.saleor.io/docs/3.x/developer/app-store/apps/klarna", | ||
| supportUrl: "https://github.com/saleor/saleor-app-payment-klarna/issues", | ||
| brand: { | ||
| logo: { | ||
| default: `${apiBaseURL}/logo.png`, | ||
| default: `${apiBaseURL}${env.NEXT_PUBLIC_BASE_PATH}/logo.png`, |
There was a problem hiding this comment.
question: Similar to above. Will this not break without the NEXT_PUBLIC_BASE_PATH?
There was a problem hiding this comment.
Yes, is true. The app should not start if those variables are not set.
src/lib/redis/RedisAPL.ts
Outdated
There was a problem hiding this comment.
question: Would it be okay for you if we moved it to App-SDK? Not blocking for this PR but in the future
There was a problem hiding this comment.
I'm okay with it
peelar
changed the title
| 18 |
There was a problem hiding this comment.
Why change to 18 and stay with 20 in the docker? 20 is latest LTS which is most likely a better choice unless theres issue with dependency
| @@ -21,7 +22,7 @@ export const paymentGatewayInitializeSessionSyncWebhook = | |||
| apl: saleorApp.apl, | |||
| event: "PAYMENT_GATEWAY_INITIALIZE_SESSION", | |||
| query: UntypedPaymentGatewayInitializeSessionDocument, | |||
| webhookPath: "/api/webhooks/saleor/payment-gateway-initialize-session", | |||
| webhookPath: `${env.NEXT_PUBLIC_BASE_PATH}/api/webhooks/saleor/payment-gateway-initialize-session`, | |||
There was a problem hiding this comment.
| webhookPath: `${env.NEXT_PUBLIC_BASE_PATH}/api/webhooks/saleor/payment-gateway-initialize-session`, | |
| webhookPath: `/api/webhooks/saleor/payment-gateway-initialize-session`, |
Urls in the webhook definitions should not be changed. The full URL is constructed in the createManifestHandler (https://github.com/saleor/saleor-app-payment-klarna/pull/65/files#diff-8b106ce88b28921d34154dd6adf1dca085721efccd424f05bc739b52c5bf72baR33). In manifest file comment provided a better solution.
| @@ -17,16 +17,16 @@ export default createManifestHandler({ | |||
| id: "app.saleor.klarna", | |||
| name: "Klarna", | |||
| about: packageJson.description, | |||
| tokenTargetUrl: `${apiBaseURL}/api/register`, | |||
| appUrl: iframeBaseUrl, | |||
| tokenTargetUrl: `${apiBaseURL}${env.NEXT_PUBLIC_BASE_PATH}/api/register`, | |||
There was a problem hiding this comment.
If the APP_API_BASE_URL url will be set, or appBaseURl will be passed by manifestFactory, this URL will be broken.
The best solution would be setting up APP_API_BASE_URL in the env.mjs file to use NEXT_PUBLIC_BASE_PATH.
| tokenTargetUrl: `${apiBaseURL}/api/register`, | ||
| appUrl: iframeBaseUrl, | ||
| tokenTargetUrl: `${apiBaseURL}${env.NEXT_PUBLIC_BASE_PATH}/api/register`, | ||
| appUrl: `${iframeBaseUrl}${env.NEXT_PUBLIC_BASE_PATH}`, |
|
|
||
| export const getAuthData = async () => { | ||
| const { apl } = saleorApp; | ||
| // ! env.SALEOR_API_URL is not required but it is type casted here |
There was a problem hiding this comment.
So if wont be provided, the app will fail silently.
SALEOR_API_URL should be then mandatory, or the code using this function should handle error.
|
|
||
| try { | ||
| const token = ""; | ||
| const client = createServerClient(env.SALEOR_API_URL as string, token); |
There was a problem hiding this comment.
Whats the purpose of adding Apollo Server to this repository? If I'm understanding it correctly, so far its only used to create gql client, and for that you can use urql which is already part of the repo (src/lib/create-graphq-client.ts)
|
|
||
| export default async function handler(req: NextApiRequest, res: NextApiResponse) { | ||
| const query = gql` | ||
| query test { |
There was a problem hiding this comment.
Would use here more meaningful name which would help recognize the source, eg
| query test { | |
| query KlarnaHealthCheck { |
Or you can use app query, which will also return information if the app token is still valid.
… failure urls support, support for app base path