Core refactor #41
Core refactor #41
Conversation
|
Damn, that's a big PR. |
| throw new Error(`Apollo Server expects exactly one argument, got ${arguments.length + 1}`); | ||
| } | ||
|
|
||
| return async (req: express.Request, res: express.Response, next) => { |
There was a problem hiding this comment.
do we really want to return an async function instead of a promise?
There was a problem hiding this comment.
As far as I know, they're interchangeable, as suggested here. Right now that gets transpiled to a promise anyway, if you look at the /dist folder.
The reason I used async/await is that it makes the code so much more readable. Compare our code to express-graphql, and you'll see what I mean.
|
@helfer is the API finished enough to start working on express/koa integrations? |
| return data ? JSON.stringify(data).replace(/\//g, '\\/') : null; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
I don't think this comment applies anymore. Should it be removed?
There was a problem hiding this comment.
Yes, you're right, it should be removed.
|
@HriBB I want to take one more day to review the code and discuss the API, but after that, I think we should be ready to go. If you have time to read the code and leave comments, that would be great! |
|
@nnance Do you think it would be possible to write a simple function for tests that instantiates a HAPI server with apollo on it, so we could reuse all the tests between HAPI and Express? I think if possible, we should make the tests reusable between the different implementations, even if it makes them a bit more complicated to write. What do you think? |
|
One thing still missing in the express implementation is strict argument checking, i.e. making sure that every argument that's passed to the middleware is an allowed argument. I'm thinking about using Joi for that, and maybe those checks could be reused in other implementations as well. And while I'm at it, I think it would be nice to add some of the badges that the Joi project uses, to make sure we keep our dependencies up-to-date and don't have any dependencies with known vulnerabilities. |
|
@helfer I would expect that to be the case and if it doesn't work then we need to improve the tests or the api. I have been working on testing the PR by building a working server based on it. I believe I have found a couple of issues but as I continue to work through the issues I will let you know what I find. I also, have a working version of hapi that includes graphiql. I will try to integrate the hapi implementation with the tests tonight. |
|
@helfer I started refactoring the expressApollo.test.ts to work with HAPI but then realized there are the testApolloServerHTTP tests. Which one do you want to be able to use both express and hapi? The reason I ask is there is a comment at in the http tests that indicates that we might should remove it completely. |
|
@HriBB I am actively working on the HAPI implementation but it would be great if you could build KOA. |
|
@nnance the HTTP tests was originally the express-graphql tests file. There's not much left of it, so I think I'll just get rid of it and copy whatever tests there still are into the other test file. |
| @@ -95,7 +95,7 @@ export function renderGraphiQL(data: GraphiQLData): string { | |||
| } | |||
| } | |||
| // We don't use safe-serialize for location, becuase it's not client input. | |||
| var fetchURL = locationQuery(otherParams, ${endpointURL}); | |||
| var fetchURL = locationQuery(otherParams, '${endpointURL}'); | |||
There was a problem hiding this comment.
@helfer I still need you to confirm the commit and the next one are needed. I needed them for the example in the readme to work
There was a problem hiding this comment.
Yeah, this one seems like a no-brainer. I'm pretty sure that wouldn't have worked for me. I think it worked when I called the safe-serialize, but then I removed it and probably didn't test again.
|
@nnance Is the coverage actually working now? When I look at the expressApollo file on coveralls, it looks like a lot of lines are neither covered nor not covered. Is that normal? @jbaxleyiii adding you to the conversation here because I think you set up coveralls for apollo client. Is that correct? |
TODO:
@nnance