-
Notifications
You must be signed in to change notification settings - Fork 13
Typescript CreatePaymentMethodLambda #7017
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
Typescript CreatePaymentMethodLambda #7017
Conversation
Size Change: 0 B Total Size: 1.49 MB ℹ️ View Unchanged
|
8a9088b
to
62d4075
Compare
support-workers/package.json
Outdated
@@ -6,13 +6,14 @@ | |||
"scripts": { | |||
"test": "jest --group=-integration", | |||
"it-test": "jest --group=integration", | |||
"lint": "eslint src/typescript/**/*.ts", | |||
"lint:check": "eslint src/typescript/**/*.ts", | |||
"lint:fix": "eslint src/typescript/**/*.ts --fix", |
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.
These match the commands in support-frontend now for consistency
6269470
to
192a611
Compare
This reverts commit 4c9d623. # Conflicts: # support-workers/package.json
477bf45
to
83d7d70
Compare
@@ -9,10 +9,11 @@ | |||
"lint": "eslint src/typescript/**/*.ts", | |||
"check-formatting": "prettier --check **.ts", | |||
"build": "tsc", | |||
"bundle": "esbuild src/typescript/lambdas/*.ts --bundle --platform=node --target=node22 --outdir=target/typescript", |
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 have introduced esbuild as a bundler because it was very difficult to get pnpm to produce a flat set of dependencies for deployment. It should also produce a smaller deployment artifact.
630469d
to
6e8fe30
Compare
import { z } from 'zod'; | ||
|
||
export const billingPeriodSchema = z.union([ | ||
z.literal('Monthly'), //TODO: share this with support-frontend |
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've decided to do all the TODOs around sharing models in a follow up PR because it will probably involve some quite extensive changes
5f409ee
to
df65194
Compare
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 think this looks good, nice work! 👏
I've left a few comments / questions, all minor stuff.
"install-runtime-dependencies": "pnpm install --ignore-scripts --modules-dir target/typescript/node_modules --production", | ||
"build-and-zip": "pnpm clean-target && pnpm build && pnpm install-runtime-dependencies && cd target/typescript && zip -r support-workers.zip ./* -x \".*\" -x \"__MACOSX\"", | ||
"package": "pnpm check-formatting && pnpm lint && pnpm test && pnpm build-and-zip" | ||
"zip": "cd target/typescript && zip -r support-workers.zip ./* -x \".*\" -x \"__MACOSX\"", |
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.
What's this exclude doing? -x \".*\"
I'm finding it hard to parse!
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.
It's dotfiles, probably not needed now that we are using esbuild actually
support-workers/package.json
Outdated
"package": "pnpm check-formatting && pnpm lint && pnpm test && pnpm build-and-zip" | ||
"zip": "cd target/typescript && zip -r support-workers.zip ./* -x \".*\" -x \"__MACOSX\"", | ||
"bundle-and-zip": "pnpm clean-target && pnpm bundle && pnpm zip", | ||
"package": "pnpm check-formatting && pnpm lint:check && pnpm test && pnpm bundle-and-zip" |
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.
Does type checking happening implicitly in one of these commands? If not should it be added?
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.
Yes it needs to be added 👍
z.literal('StripeApplePay'), | ||
z.literal('StripePaymentRequestButton'), |
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.
Any reason not to extract these out to named const
s like the others?
directDebitPaymentGatewaySchema, | ||
z.literal('Amazon Pay - Contributions USA'), | ||
]) | ||
.or(stripePaymentGatewaySchema); |
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.
What does the .or
do here? Is the result the same as adding stripePaymentGatewaySchema
to the union?
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.
Yes in this case it actually is, although it isn't in all cases 👍
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.
If fact this schema wasn't actually used
const supporterPlus = createPaymentMethodStateSchema.parse( | ||
createPaymentSupporterPlus, | ||
); | ||
expect(supporterPlus.product.currency).toBe('EUR'); | ||
expect(supporterPlus.acquisitionData?.ophanIds.pageviewId).toBe( | ||
'9999999999999', | ||
); |
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.
What do you think about splitting each of these product/payment method tests out to their own test/it block? I think it's helpful as it makes it clearer what's going on if there are failures.
), | ||
); | ||
} | ||
return Promise.reject(new Error('Unknown payment method type')); |
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.
Is it worth including the unknown type in the error? Might be helpful to know what it was if we ever reach this line.
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.
This is actually unreachable code, I'm surprised it wasn't being flagged by lint
Seen on PROD (merged by @rupertbates 8 minutes and 47 seconds ago)
Sentry Release: support-client-side, support |
"lint:check": "eslint src/typescript/**/*.ts", | ||
"lint:fix": "eslint --fix src/typescript/**/*.ts", | ||
"check-formatting": "prettier --check **.ts", | ||
"build": "tsc", | ||
"bundle": "esbuild src/typescript/lambdas/*.ts --bundle --platform=node --target=node22 --outdir=target/typescript", | ||
"clean-target": "rm -rf target && mkdir -p target", | ||
"install-runtime-dependencies": "pnpm install --ignore-scripts --modules-dir target/typescript/node_modules --production", | ||
"build-and-zip": "pnpm clean-target && pnpm build && pnpm install-runtime-dependencies && cd target/typescript && zip -r support-workers.zip ./* -x \".*\" -x \"__MACOSX\"", | ||
"package": "pnpm check-formatting && pnpm lint && pnpm test && pnpm build-and-zip" | ||
"zip": "cd target/typescript && zip -r support-workers.zip ./* -x \".*\" -x \"__MACOSX\"", |
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.
is there a standard for what these commands should be?
in support service-lambdas we use type-check, lint or lint --fix and check-formatting or fix-formatting
Obviously I'm not working on support-workers much now, but it is already a bit of a pain to remember the commands so it would be nice if they could be consistent across projects
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.
Yes I agree, I might make a health card to standardise them
Name: `/${stage}/support/support-workers/${configKeyName}`, | ||
WithDecryption: true, | ||
}; | ||
const command = new GetParameterCommand(params); |
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'm actually doing a PR in support-service-lambdas (following the threads on chat) to add a module to retrieve all the separate config keys under a /stage/stack/app/* in one go and parse it as json as a whole.
It's interesting that you've gone for storing the whole config as JSON in a single key though, is that preferred over individual keys? As the idea is the module would encourage our standard supporter revenue way.
export async function getConfig<I, O, T extends z.ZodType<O, z.ZodTypeDef, I>>( | ||
stage: Stage, | ||
configKeyName: string, | ||
schema: T, | ||
): Promise<O> { |
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.
why <I, O, T extends z.ZodType<O, z.ZodTypeDef, I>>
with schema: T,
and not <O>
with schema: z.ZodType<O, z.ZodTypeDef, any>
? ( @tjmw and I were looking at similar code and wondering this morning )
What are you doing in this PR?
This PR migrates the CreatePaymentMethod lambda to Typescript it follows on from #7008 and is part of the work to fully migrate support-workers from Scala to Typescript.
Trello Card
Why are you doing this?
Things to test