-
Notifications
You must be signed in to change notification settings - Fork 7
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
Scaffold new v1 api with incremental approach #259
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
✨ No issues found! Your code is sparkling clean! ✨ |
@@ -0,0 +1,565 @@ | |||
{ |
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.
@pellicceama initial spec here.
.meta({ | ||
openapi: {method: 'GET', path: '/connection'}, | ||
}) | ||
.input(z.void()) |
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.
should we include pagination?
a37d76c
to
80db9c3
Compare
80db9c3
to
d4990a1
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.
❌ Changes requested. Reviewed everything up to c115f75 in 2 minutes and 48 seconds
More details
- Looked at
1418
lines of code in16
files - Skipped
1
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. packages-v1/api-v1/app.ts:10
- Draft comment:
Typo: Change 'tos specify url' to 'to specify url'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the typo exists, it's in a comment rather than in actual code. The meaning is still clear despite the typo. Comments are for developer documentation and this minor typo doesn't impact code functionality or significantly impact readability. According to the rules, we should not make comments that are obvious or unimportant.
The typo could be seen as reducing code quality and professionalism. Some teams might have strict standards for documentation quality.
While documentation quality matters, this is such a minor issue that fixing it provides negligible value and creates unnecessary PR noise.
Delete this comment as it points out an unimportant typo in a comment that doesn't significantly impact code quality or understanding.
2. packages-v1/api-v1/trpc/routers/index.ts:41
- Draft comment:
Review the mergeRouters call formatting – while trailing commas are allowed, consider if a more consistent style would improve readability. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
3. packages-v1/api-v1/app.ts:10
- Draft comment:
Typo in comment: 'tos' should be 'to'. - Reason this comment was not posted:
Marked as duplicate.
4. packages-v1/api-v1/app.ts:20
- Draft comment:
The mounting strategy differs between TRPC and OpenAPI handlers ('/v1/trpc' vs '/v1*'). Consider unifying the mounting approach to reduce confusion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The author is already aware of the inconsistency and has documented why it exists. The comment suggests unifying the approach, but the code comments explicitly state that other approaches don't work when mounted inside Next.js. This suggests the inconsistency is unavoidable given the current constraints.
Maybe there's a newer version or configuration of these libraries that would allow for consistent mounting patterns that the author hasn't discovered?
The code comments indicate this has been thoroughly investigated, and the current solution is the only working approach when mounted in Next.js. The inconsistency is a known limitation.
Delete the comment as it's suggesting a change that's already been investigated and documented as not possible in the current setup.
Workflow ID: wflow_JSr6cj02QhUZO071
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
import {contextFromRequest} from './context' | ||
import {appRouter} from './routers' | ||
|
||
export const createTrpcHandler = |
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 functionality already exists - consider using createRouterTRPCHandler
instead. It's more flexible as it allows passing different routers.
- function
createRouterTRPCHandler
(createRouterHandler.ts)
import type {paths} from '@openint/api-v1/__generated__/openapi.types' | ||
|
||
export const trpcClient = createTRPCClient<AppRouter>({ | ||
links: [httpLink({url: '/api/trpc'})], |
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.
TRPC client URL is inconsistent with the mounted route. It uses '/api/trpc' but should likely be '/api/v1/trpc' matching app.ts.
links: [httpLink({url: '/api/trpc'})], | |
links: [httpLink({url: '/api/v1/trpc'})], |
|
||
export const createTrpcHandler = | ||
(opts: {endpoint: `/${string}`}) => (req: Request) => { | ||
console.log('handleTrpcRequest', req.url) |
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.
Consider gating or reducing verbose logging in production; logging every request in handlers may leak information or clutter logs.
console.log('handleTrpcRequest', req.url) | |
if (process.env.NODE_ENV !== 'production') console.log('handleTrpcRequest', req.url) |
This reverts commit b6baee1.
Having problems with deploy. But if you run it locally then visit http://localhost:4000/api/v1 you'l be able to see the new api spec explorer.
Important
Scaffold new v1 API with OpenAPI and tRPC integration, including routes, handlers, tests, and client setup.
packages-v1/api-v1
./connection
,/connector-config
, and/health
inapp.ts
.Elysia
framework with OpenAPI and tRPC integration.scripts/gen-oas.ts
andtrpc/openapi.ts
.models.ts
forplaid
andgreenhouse
connectors.trpc/handlers.ts
for OpenAPI and tRPC requests.app.spec.ts
for route verification and client interactions.sdk-v1/index.ts
.package.json
inapi-v1
andsdk-v1
with necessary dependencies.This description was created by
for c115f75. It will automatically update as commits are pushed.