-
Notifications
You must be signed in to change notification settings - Fork 57
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
Improve api typing #129
Improve api typing #129
Conversation
api/src/db/dbConnection.ts
Outdated
@@ -6,7 +6,7 @@ import { chainModels, getChainModels, userModels } from "@shared/dbSchemas"; | |||
import { Template, TemplateFavorite, UserAddressName, UserSetting } from "@shared/dbSchemas/user"; | |||
import { chainDefinitions } from "@shared/chainDefinitions"; | |||
|
|||
const csMap = { | |||
const csMap: { [key: string]: string } = { |
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 needed to be this flexible? if you skip this type it's implicitly typed as object with specified keys and values. It seems that what really needed here is typed 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.
I removed the type and added a typeguard instead. But yeah ideally we would have env var typing & validation.
api/src/routes/v1/graphData.ts
Outdated
|
||
const route = createRoute({ | ||
method: "get", | ||
path: "/graph-data/{dataName}", | ||
tags: ["Analytics"], | ||
request: { | ||
params: z.object({ | ||
dataName: z.string().openapi({ example: "dailyUAktSpent", enum: authorizedDataNames }) | ||
dataName: z.string().openapi({ example: "dailyUAktSpent", enum: Array.from(AuthorizedGraphDataNames) }) |
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.
Array.from
is already a hack here needed to overcome as const
cast. To me it would be the same in terms of code being clean to do:
AuthorizedGraphDataNames as unknown as string[]
However I'd just created a dedicated type in the source instead of deriving type from an array. Yes, some duplication but ts would enforce to follow and it works consistently without hacks
type AuthorizedGraphDataName = (
"dailyUAktSpent" |
"dailyUUsdcSpent" |
"dailyUUsdSpent" |
"dailyLeaseCount" |
"totalUAktSpent" |
"totalUUsdcSpent" |
"totalUUsdSpent" |
"activeLeaseCount" |
"totalLeaseCount" |
"activeCPU" |
"activeGPU" |
"activeMemory" |
"activeStorage"
)
export const authorizedGraphDataNames: AuthorizedGraphDataName[] = [
"dailyUAktSpent",
"dailyUUsdcSpent",
"dailyUUsdSpent",
"dailyLeaseCount",
"totalUAktSpent",
"totalUUsdcSpent",
"totalUUsdSpent",
"activeLeaseCount",
"totalLeaseCount",
"activeCPU",
"activeGPU",
"activeMemory",
"activeStorage"
];
// avatarUrl: validatorFromDb?.keybaseAvatarUrl | ||
// }; | ||
} | ||
// const proposer = null; // TODO: 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.
is there a reason we can't remove this code? I know it's here only because of prettier so just asking
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.
We have kept it in hope of fixing it one day. There is an issue with proposals created during genesis. I created #132 to keep track of it.
@@ -75,7 +75,22 @@ export const getDashboardData = async () => { | |||
}; | |||
}; | |||
|
|||
export const AuthorizedGraphDataNames = [ | |||
type AuthorizedGraphDataName = |
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.
❤️
noImplicitAny
in tsconfig