-
Notifications
You must be signed in to change notification settings - Fork 14
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
Maintenance: Lambda Typescript Foundations #3429
base: maintenance/lambda-refactor
Are you sure you want to change the base?
Maintenance: Lambda Typescript Foundations #3429
Conversation
import { createLiquidationAddress, getUser } from '../graphql'; | ||
import { AppSyncResolverEvent, HandlerContext } from '../types'; | ||
import { Maybe } from 'graphql/jsutils/Maybe'; | ||
import { KycStatus, User } from '../../../../../../src/graphql/generated'; |
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.
A MEGA relative import. Yuck. I know. But I'll probably deal with this in another PR if I really have to.
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.
Yeah would be great if it's possible to add an @amplify alias or something!
scripts/create-data.js
Outdated
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 some of this left over from testing? Not sure why there are changes to the number of default colonies, removing Amy and Fry, etc
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.
Definitely left over! 👀 I wanted to make the create-data script faster 🏃 I'll make sure to update this before I officially open this for a proper review!
scripts/watchAmplifyFiles.js
Outdated
const watchDir = './amplify'; | ||
|
||
const watcher = chokidar.watch(watchDir, { | ||
ignored: /^(?!.*\.env$).*\/\.[^/]+/, // Ignore dotfiles except .env |
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.
Can we ignore files inside of the node_modules folder here too if it is only a small addition? 👀
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.
Ah yes thanks @iamsamgibbs I forgot about this suggestion of yours!
package.json
Outdated
@@ -40,7 +40,9 @@ | |||
"forward-time": "node scripts/forward-time", | |||
"playwright:install": "npx playwright install --with-deps", | |||
"playwright:run": "playwright test", | |||
"playwright:watch": "playwright test --ui" | |||
"playwright:watch": "playwright test --ui", | |||
"watch:lambdas": "node scripts/transpile-lambdas.js", |
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 file scripts/transpile-lambdas.js
doesn't exist?
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.
Thank you! Left over from previous implementation 🙃
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'll be leaving it in after all after reverting back to a previous implementation 😄
0ac103a
to
d3b4107
Compare
638f510
to
9f96d81
Compare
4914b32
to
cfb6bfe
Compare
cfb6bfe
to
ab52415
Compare
Description
Note
It doesn't make sense to get this reviewed until we can first verify if it's still going to work properly once we deploy it to our QA environment.
I believe there's also a chance that this might be rendered obsolete due to some upcoming ops work. @bogdan-1337 or @rdig can tell you more about it.
And so this will be in "Draft" mode until we come up with a final decision.
This PR introduces Typescript to the
bridgeXYZMutation
lambda. The goal is to consider this as our standard when converting other existing lambdas into Typescript as well as for new ones.With this setup, you should just be able to focus on building/maintaining the typescript files in a lambda, taking advantage of type safety and other goodies that come with TypeScript while esbulid transpiles your changes in realtime.
Key Things To Note
Why I'm using
esbuild
The esbuild script can be found in
scripts/transpile-lambdas.js
.There are two
package.json
scripts you can use to conveniently run this:Why I added the
source-map-support
packageimport 'source-map-support/register';
at the top of the TypeScript entry file (index.ts) and it will just work 😄Sections marked with
// @TODO: Lambda utils refactor candidate
My next task is to create an npm package that hosts all of our Lambda shared utils. So sections marked with this will be scrapped and moved to that npm package as soon as that's available. There's possibly more of them hidden somewhere but I'll cross that bridge when I get there.
Testing
We will have to test all features of the app that rely on the
bridgeXYZMutation
lambda. Some of the key things to test would be these places:Crypto to fiat page
URL: http://localhost:9091/account/crypto-to-fiat
Crypto to fiat tab in the UserHub component
TODO
Resolves #3187