-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: added Express
request handler
#7
Conversation
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
packages/backend/src/util/handlers/createExpressSynthqlHandler.test.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/util/handlers/createExpressSynthqlHandler.test.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/util/handlers/createExpressSynthqlHandler.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/util/handlers/createExpressSynthqlHandler.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/util/handlers/createExpressSynthqlHandler.ts
Outdated
Show resolved
Hide resolved
I don't understand the reasoning for this @jimezesinachi. How else do you know if it's working properly? |
What I'm saying is, all the current tests work and pass, and any commented code is work in progress (and hence you don't need to point them out, similar to what I mentioned last week). |
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
import { from } from '../../tests/generated.schema'; | ||
import { DB } from '../../tests/db'; | ||
|
||
describe('createExpressSynthqlHandler', () => { |
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 test isn't really testing anything useful @jimezesinachi
A few of the important invariants you should be testing:
- The response is always 1..n lines of JSON
- when returnLastOnly the status code can vary depending on the response kind
- There's really no need to mock: spin up an actual server using
http.createServer
- Tests for error cases i.e. when the query is malformed
- Add also tests using the react client. Notice that there is already a http.createServer in /react, instead of the custom handler code refactor to use the createExpressSynthqlHandler
packages/backend/src/util/handlers/createExpressSynthqlHandler.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/util/handlers/createExpressSynthqlHandler.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
...req, | ||
headers: req.headers, | ||
body: body, | ||
} as IncomingMessageWithBody; |
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.
don't do as Type
, this is generally unsafe behaviour. Instead do
const newReq: 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.
This would be ideal, but the goal here is actually type casting. We don't have all the values for the type, so using a type annotation in this case, causes a type mismatch error.
packages/backend/src/util/handlers/createExpressSynthqlHandler.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
SynthQL pull request template
Why
Adding an Express request handler to allow easier setup and usage in Express applications
What changed
A single handler function and unit tests
Important
Tests are still very rudimentary at this point. All commented code is failing tests that I'm working on, and is there intentionally, until final review is requested.