-
Notifications
You must be signed in to change notification settings - Fork 0
🌎 Set up common API types #85
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
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/loo-labs/waterpark/EAvrmtegycbcJATp6pLMcpR1DW2b |
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.
You're a Typescript wizard Justin 🧙♂️🔮
common/src/types.ts
Outdated
|
||
type Methods = Readonly<Partial<Record<MethodName, MethodSchema>>> | ||
type Children = Readonly<Partial<Record<Exclude<ChildPath, RootPath>, EndpointSchema>>> | ||
export interface EndpointSchema extends Methods { |
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.
export interface EndpointSchema extends Methods { | |
export interface APISchema extends Methods { |
I think APISchema
is a better name for this. I also think we should change the name of the generic Endpoint
being used in the definition of Router
up above to API
as well.
It just makes more sense as we construct a router with an api
.
const router = new Router(api)
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 API makes sense for the root-level object, but what about something like /clubs? It makes more sense (to me, at least) to call that an endpoint
Maybe I can create a typealias APISchema = EndpointSchema
so we get the best of both names 🤔
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.
But this interface describes our entire api, not just a single endpoint. It would make more sense to name the actual endpoint EndpointSchema
.
Wait..are EndpointSchema
and Children
's definition mutually recursive?
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.
Wait..are
EndpointSchema
andChildren
's definition mutually recursive?
Yep! The /clubs
endpoint is also a fully-fledged Endpoint schema. /clubs/:id
would be yet another layer in, but it also has an Endpoint schema. Altogether, it forms a tree, with our base API as the root.
That's why it's challenging to come up with a good name for things. I wouldn't really call the root API an "endpoint", but it quacks and walks like one. /clubs/:id
sounds much more like an endpoint to me.
Since I never formally learned web dev, I don't really know what the proper terminology is. Are /
and /clubs
and /club/:id
"endpoints" or "APIs"?
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.
Since I never formally learned web dev, I don't really know what the proper terminology is. Are / and /clubs and /club/:id "endpoints" or "APIs"?
My definition is the "API" is the structure, grouping, and schema of all "endpoints".
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.
Ok, then we can stick to "Endpoint".
Following your definition, maybe this makes sense:
type API = {
root: Endpoint
}
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 I like that!
common/src/demo.ts
Outdated
const API = { | ||
methods: { | ||
get: { | ||
response: { | ||
body: t.literal('Water water water, loo loo loo'), | ||
}, | ||
}, | ||
}, | ||
children: { | ||
'/clubs': clubsAPI, | ||
}, | ||
} |
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 would be clean if we could extract the body of the root endpoints into an API as well.
const API = {
root: rootAPI,
children: {
'/clubs': clubsAPI,
'/users': usersAPI
}
}
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 rootAPI
object look like?
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.
Same thing as clubsAPI
, this would require changing the definition of EndpointSchema
const rootAPI = {
methods: {
get: {
response: {
body: t.literal('Water water water, loo loo loo'),
},
},
},
}
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.
So we're just moving .methods
into .root.methods
? This is adding more nesting on top of what is already too much nesting
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 adds more nesting and makes the type defs more messy, but I'd argue the overall shape and readability of our API
object will be cleaner and easier to grok. I'll let you make the final decision on this one though.
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.
Maybe we only do it on the root API object, as suggested here: #85 (comment)
Then we have
const rootEndpoint = {
methods: {
get: ...
}
children: {
'/clubs': clubsEndpoint
}
}
const api = {
root: rootEndpoint
}
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 sounds good.
common/src/express/types.ts
Outdated
} | ||
|
||
export interface TypedRouter<Endpoint extends Schema.Endpoint> { | ||
all: TypedMatcher<Endpoint, this, 'all'> |
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.
Matcher
in the name TypedMatcher
doesn't make any sense re: web development and API servers. The names of types that include API
, Endpoint
, Router
etc make sense. TypedMatcher
is too abstract, can we come up with a better name for this 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.
It comes from Express itself, where the type is called RouterMatcher
. I agree it's quite obscure, but I can't think of a better name. After all, it does match a path and direct control to a handler.
common/src/express/router.ts
Outdated
class TypedAPIError extends Error {} | ||
class FrozenRouterError extends TypedAPIError { | ||
constructor() { | ||
super('Cannot modify a frozen router.') | ||
} | ||
} | ||
// @ts-ignore unused | ||
class UnhandledPathError extends TypedAPIError { |
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 do we need this intermediary class of TypedAPIError
along the class hierarchy chain? Can we just have FrozenRouterError
and UnhandledPathError
extend directly from Error
?
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.
At the moment it's functionally the same, but imagine if we want every API type runtime error to do something common (e.g. print out the endpoint). Then having that base class there would be useful. I'm just jumping the gun and doing it in advance.
common/src/express/router.ts
Outdated
public freeze(): this { | ||
if (this.frozen) throw new FrozenRouterError() | ||
this.frozen = true | ||
return this | ||
} |
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 do we need to freeze a Router? Is this to support statically checking if we have a handler for each of our API endpoints?
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 trying to model (in runtime, for now) what we might eventually do in compile time. After you freeze a router, you shouldn't be able to modify it. A file might define a clubRouter
and export clubRouter.freeze()
, so that other files that import it can't modify it.
common/src/api-types.ts
Outdated
|
||
export type Path<Endpoint extends Schema.Endpoint> = Schema.Path & | ||
(Endpoint extends { methods: Schema.Methods } | ||
? keyof Endpoint['children'] | Schema.RootPath |
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 Endpoint
(the API Schema) is the root object aka Endpoint extends { methods: Schema.Methods }
is true, why is the second value of the turnury expression the |
of keyof Endpoint['children']
and Schema.RootPath
?
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.
So this is the weird thing about Endpoint
. The .methods
field corresponds to the methods available at the /
endpoint. The .children
field corresponds to the endpoints for the children. It's messy, but the /
endpoint is fundamentally different from all the others in the sense that it can't have any further children.
Schema.RootPath
is /
, so this just means "if the endpoint has a .methods
, then it supports the /
path as well as all of its children"
common/src/express/router.ts
Outdated
return this | ||
} | ||
|
||
public all<Path extends Get.Path<Endpoint> & PathParams>( |
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 do all the generic Path
s in the Router
s all
/get
/post
/etc extend Get.Path
? Do we have to create a different namespace for all the different http verbs?
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.
So Get
is the (possibly misnamed, open to suggestions) namespace for the utility that gets things from endpoints.
Get.Path<Endpoint>
is the type that represents all paths you can take from Endpoint
. It doesn't have anything to do with the HTTP verb GET
.
common/src/api-types.ts
Outdated
|
||
export type Path = `/${string}` | ||
export type RootPath = '/' | ||
export type Children = Readonly<Partial<Record<Exclude<Path, RootPath>, Endpoint>>> |
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 do we need Partial<T>
here?
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 not sure actually, it's to signify that not every Path
is guaranteed to be in .children
. I'm not sure if this happens by default in Record
, I didn't check
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.
Did some looking into it; the code works without the Partial
here. However, if you remove Partial
from the definition of the similar Methods
type, it won't work. If your type has a finite number of values (like MethodName
), then Typescript expects every value to be a property in the corresponding record. Removing the Partial
in the definition of Children
works because there are infinitely many possible Path
s.
tl;dr: if you want a record type where all the keys are optional, Partial<Record<K, V>>
always works. Depending on K
, you can sometimes get away with just doing Record<K, V>
. To be safe, stick to always wrapping a Partial
around your type, so that way your type definition works regardless of K
.
15364e4
to
b1efa0b
Compare
Combine API types into two simple namespaces Add the .use method to routers Freeze routers to declare that we're done adding handlers
b1efa0b
to
b464491
Compare
Purpose
Closes #42
We want a canonical representation of our API that both the frontend and backend can use. It's quite easy to write down a schema for our API. It's a bit harder get the frontend/backend to use it. The scope of this PR will be limited to switching our backend over to something that uses those types.
Currently, we have the custom
Router
built, but our backend doesn't use it yet. One of the reasons it's not set up is because I'm too lazy to actually write the schemas for our APIsApproach
Idea was inspired by Gary Bernhardt, but @jeffzh4ng and I couldn't figure out his implementation for the life of us. Instead we substituted a system of our own design. Maybe ours is better? We'll see about that when things are fully in place.
Alternatives? Here's one: https://hackernoon.com/express-fp-an-express-wrapper-for-type-safe-request-handlers-f8c411cc4a7b
We use
io-ts
instead of Joi, which will eventually enable clean and easy (de)serialization. This seems to be a staple of anyone who tries to make shared types a thing.Testing
TODO
Open Questions and Pre-Merge TODOs
Learning
Describe the research stage
Lots of extremely arcane Typescript went into this. The official Typescript docs were the main source of information there.
Links to blog posts, patterns, libraries or addons used to solve this problem
Blog Posts