Skip to content
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

Merged
merged 6 commits into from
May 9, 2024
Merged

feat: added Express request handler #7

merged 6 commits into from
May 9, 2024

Conversation

jimezesinachi
Copy link
Contributor

@jimezesinachi jimezesinachi commented May 2, 2024

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.

Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
@fhur
Copy link
Collaborator

fhur commented May 3, 2024

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.

I don't understand the reasoning for this @jimezesinachi. How else do you know if it's working properly?

@jimezesinachi
Copy link
Contributor Author

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.

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', () => {
Copy link
Collaborator

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:

  1. The response is always 1..n lines of JSON
  2. when returnLastOnly the status code can vary depending on the response kind
  3. There's really no need to mock: spin up an actual server using http.createServer
  4. Tests for error cases i.e. when the query is malformed
  5. 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

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;
Copy link
Collaborator

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 = { ... }

Copy link
Contributor Author

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.

Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
@fhur fhur merged commit b6e4f6e into master May 9, 2024
1 check passed
@jimezesinachi jimezesinachi deleted the feat/express-handler branch May 17, 2024 09:42
fhur pushed a commit that referenced this pull request May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants