DO NOT MERGE YET - Feature/restify docs tests

[joelgriffith]

Hey folks, fine project here. This is my first PR, as well as my first TS undertaking, so take that into consideration :)

This introduces a new interface for restify, that almost is a copy/pasta of the express integration. I've also taken the liberty of adding some dependencies for this so that things compile and tests run.

Let me know changes you'd like, and whatever else I should take into consideration... and thanks!

• Merge after Monorepo (WIP) #164

[apollo-cla]

@joelgriffith: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[stubailo]

that almost is a copy/pasta of the express integration

What's different about it? I've heard of people using apolloExpress successfully with Restify.

[joelgriffith]

I initially didn't think this would work OOB as the naming suggested it's for express only. There's currently an issue here that this would close as well: #125

This achieves a certain level of integration against restify by documenting it and adding tests. To be fair: I haven't explored the internals of testSuite(createApp);, so I'm not sure what it's buying as far as regressions go.

Bascially: instead of folks searching for how to use this with restify, apollo-server comes out and says that it supports restify.

[stubailo]

Sure, I'm just wondering if we could do the same by just saying export const apolloRestify = apolloExpress - what's the difference in the implementation?

[joelgriffith]

Sure, there's only two differences in implementation at this point

• Reference Types from @type/restify as opposed to @types/express. If one framework diverges, I'd hope that tests and type annotations would better catch the change as opposed to consumers informing this project.
• Errors messaging: 'POST body missing. Did you forget "server.use(restify.bodyParser())"?', which is restify specific.

I'm going to continue playing with the implementation locally to see if there's improvements or other considerations to fix, but wanted to get an MVP for this review to get discussion going

[joelgriffith]

Apologies on thrashing the pushes, found a few issues when consuming this from an outside project

[joelgriffith]

Doing this makes sure that the interfaces are properly exports as opposed to referencing them directly, so tests a few things in one :)

[DxCx]

hi @joelgriffith i think that's a good idea thanks for the contributation!
no worries about messy commits btw, worst case it can be just merged-commit.

also, @helfer, can we postpone merging until monorepo is merged?
this will be a new package on top of the monorepo and it will be abit messy to rebase upon it.
and, i think that after monorepo is done, it will be eaiser to do code reuse with different typings.

[helfer]

can we postpone merging until monorepo is merged?

Sure, I've marked the PR accordingly.

[joelgriffith]

I don't mind acting as custodian for this once monorepo is in place. And I definitely echo the sentiment that this is pretty repetitive, and hopefully having clear distinction now makes potential breaking changes easier in the future.

Lastly, I'll be at the GraphQL summit coming up, and would love say my hellos with this fine team. Thanks!

[stubailo]

Lastly, I'll be at the GraphQL summit coming up, and would love say my hellos with this fine team. Thanks!

Awesome! Excited to see you there!

[rricard]

@stubailo I generally agree with @joelgriffith. Express & Restify are very similar but they are effectively different. For now, apolloExpress uses a subset of express features common to restify but let's say in the future, we need to use a super fancy express feature, not available in restify under the same API, then, we're not good!

[rricard]

This is pretty cool! Not having those tests was not a great feeling. Thanks for tackling the issue, you did a great job!

[rricard]

Any help needed to add more tests ? For now, you only ensure the server is not crashing (which is a good start!).

[DxCx]

For now, apolloExpress uses a subset of express features common to restify but let's say in the future, we need to use a super fancy express feature, not available in restify under the same API, then, we're not good!

and yet, IMO, we need to avoid code duplication as much as we can,
at the moment we do not have much difference, and that's why i suggested to lean over each other.
restify will get it's own flavor, which in the future can be changed if needed.

[rricard]

@DxCx Well I understand that. If we go that way however, we definitely need more tests.

[DxCx]

i havn't spoken about testing but implementation itself, IMO, testing is always welcome :)
and ofcourse there should be separated testing for both..

[rricard]

@DxCx We completely agree 😉 , I understand that you don't want to avoid duplicating only the implementation! Just saying in this case, we need to ensure that the express implementation stays compatible with restify. And if this implementation were to break, we should then duplicate the code!

[joelgriffith]

I can easily add an alias in the export manifest to use the express integration, but just export an additional interface. I think that seems to be the best thing to do at this juncture since there's some concern about duplication.

[rricard]

@joelgriffith You will definitely need to export a new interface. I think we found a good compromise with this though! Thanks for contributing anyway, I can't wait to see what you'll do!

[joelgriffith]

Sounds good, I'll hold off then atm since I'm fighting the TS type system when trying to alias...

[joelgriffith]

Going to re-implement this in monorepo land as I'm having some rought ts conflicts after having migrated.