Adding in restify integration, docs, tests more... #189

joelgriffith · 2016-10-24T18:06:18Z

Adds in restify, some new tests (would like to move these to a shared util and test in each integration). I've went through as much md files as I could to update fw support, but may have missed a place or two.

Let me know your thoughts!

Closes #125

joelgriffith · 2016-10-24T19:32:19Z

Appears coveralls isn't yet working. Here's the coverage stats after this PR:

=============================== Coverage summary ===============================
Statements   : 98.21% ( 660/672 )
Branches     : 89.46% ( 297/332 )
Functions    : 100% ( 151/151 )
Lines        : 99.06% ( 631/637 )
================================================================================```

joelgriffith · 2016-11-07T16:02:21Z

Hey folks, this has been sitting for a few weeks, any thoughts/concerns/comments?

stubailo · 2016-11-07T23:04:01Z

I'll take a look at it soon! Thanks for the reminder.

helfer · 2016-11-29T10:53:41Z

@joelgriffith Thanks, it looks good at first glance! What were the necessary changes compared to the express version? It might be helpful to point those out in comments in the code, because most of it looks very similar.

helfer · 2016-12-07T14:04:21Z

@joelgriffith let's get this merged some time soon! I think we're pretty close, so it would be a shame if we dropped it now.

jadkap · 2017-01-10T14:47:23Z

Hi. Any idea when this PR might get merged? I understand it's possible to use the express variant with restify, but it would be cleaner to have a restify variant out of the box. I've pulled down @joelgriffith's fork to try locally (though running into a schema issue, but that's most likely because I'm new to GraphQL). Thanks.

DxCx · 2017-01-12T09:30:41Z

Hi @jadkap, im happy to see you want to take over this,
if you need any help (running into schema issues for example ;)) feel free to consult on slack (@HagaiCo)

jadkap · 2017-01-17T14:13:06Z

Haha @DxCx, did I say I wanted to take this over? I will see, given time constraints.

DxCx · 2017-01-17T23:19:42Z

well @jadkap, you already downloaded it locally to try it off, if you make it work, why not contribute it back ;)
and if you didn't make it work, as i said, you are welcome to reach me for consultant over slack :)

joelgriffith · 2017-01-17T23:21:04Z

Hey folks, sorry, been tied up.

I'm lost on where this is... I only see a conflict in CHANGELOG.md, I can fix whatever and push?

DxCx · 2017-01-18T08:00:37Z

@joelgriffith , you better hold on for #180 which will be merged any time now,
after you will rebase upon it, your integration code will look much much smaller.

jadkap

Also, after #180 rebase, it might be best to refactor index out to 2 files (index and restifyApollo) and rename test file to restifyApollo.test.ts, to be more consistent with other variants.

jadkap · 2017-01-18T14:54:41Z

README.md

+
+server.post('/graphql', graphqlRestify({ schema: myGraphQLSchema }));
+
+sever.get('/graphiql', graphiqlRestify({ endpointURL: '/graphql' }));


jadkap · 2017-01-18T14:55:29Z

packages/graphql-server-restify/package.json

+    "typed-graphql": "^1.0.2"
+  },
+  "peerDependencies": {
+    "graphql": "^0.6.1 || ^0.7.0"


add || ^0.8.0

DxCx · 2017-01-19T17:30:23Z

0.5.1 branch is merged :)
@jadkap / @joelgriffith can one of you take over and rebase / align so we can merge?

DxCx · 2017-01-19T17:31:54Z

README.md

+
+server.use(restify.bodyParser());
+
+server.post('/graphql', graphqlRestify({ schema: myGraphQLSchema }));


should be use as now apollo supports GET requests as well. :)

joelgriffith · 2017-01-19T17:39:42Z

I'll take a stab at it later today, will push once everything is resolved

DxCx · 2017-01-19T18:24:47Z

great @joelgriffith thanks! 👍

…ntegration

joelgriffith · 2017-01-22T19:59:10Z

Pushed my rebase and fixes. I'm not able to complete an install or run tests, so I'm not sure what the issue is:

# jgriffith @ jgriffith in ~/Griffin/graphql-server on git:feature/restify-integration|MERGING x [11:46:51] C:1
$ npm i

> undefined postinstall /Users/jgriffith/Griffin/graphql-server
> lerna bootstrap

Lerna v2.0.0-beta.30
Bootstrapping 8 packages
Installing external dependencies
Errored while running BootstrapCommand.execute
npm ERR! Darwin 15.6.0
npm ERR! argv "/Users/jgriffith/.nvm/versions/node/v6.9.1/bin/node" "/Users/jgriffith/.nvm/versions/node/v6.9.1/bin/npm" "install" "@types/restify@^2.0.33" "graphql-server-integration-testsuite@^0.4.3" "restify@^4.1.1" "typed-graphql@^1.0.2" "graphql-server-core@^0.4.3"
npm ERR! node v6.9.1
npm ERR! npm  v3.10.8
npm ERR! code E404

npm ERR! 404 Registry returned 404 for GET on https://registry.npmjs.org/graphql-server-integration-testsuite
npm ERR! 404
npm ERR! 404  'graphql-server-integration-testsuite' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it (or use the name yourself!)
npm ERR! 404
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, http url, or git url.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/jgriffith/Griffin/graphql-server/packages/graphql-server-restify/npm-debug.log

DxCx

Thanks @joelgriffith
Ive commented in the code found some issues
Typings issue will probably fix the compilation for you..

DxCx · 2017-01-23T04:28:13Z

packages/graphql-server-restify/src/restifyApollo.ts

+  };
+}
+
+function isOptionsFunction(arg: GraphQLOptions | RestifyGraphQLOptionsFunction): arg is RestifyGraphQLOptionsFunction {


I think this function here can be removed
It moved to core.. please check that ;)

DxCx · 2017-01-23T04:28:29Z

packages/graphql-server-restify/tsconfig.json

+        "node_modules/@types"
+    ],
+    "types": [
+        "typed-graphql",


Should be removed

DxCx · 2017-01-23T04:29:09Z

packages/graphql-server-restify/package.json

+    "@types/restify": "^2.0.33",
+    "graphql-server-integration-testsuite": "^0.4.3",
+    "restify": "^4.1.1",
+    "typed-graphql": "^1.0.2"


Should be replaced with @types/graphql

DxCx · 2017-01-23T04:29:22Z

packages/graphql-server-restify/package.json

+    "graphql": "^0.6.1 || ^0.7.0 || ^0.8.0"
+  },
+  "optionalDependencies": {
+    "@types/restify": "^2.0.33"


Should add @types/graphql

DxCx · 2017-01-23T04:30:17Z

packages/graphql-server-restify/src/restifyApollo.test.ts

+    server.get('/graphiql', graphiqlRestify( options.graphiqlOptions ));
+  }
+  server.post('/graphql', graphqlRestify( options.graphqlOptions ));
+  server.put('/graphql', graphqlRestify( options.graphqlOptions ));


Why put and not use?

Can't namespace a .use call: restify/node-restify#289

joelgriffith · 2017-01-23T17:49:58Z

OK! Types are fixed, as are tests, and README. The interesting thing to note is that restify doesn't support namespacing of the .use API, so you have to verbosely do a .get and a .post for query parsing and json parsing to work. README shows correct usage (and tests do as well).

DxCx · 2017-01-24T11:54:36Z

@helfer please merge #274 which should fix the travis issues,
then merge this upon this should give you a branch that passes tests.

helfer · 2017-01-24T23:04:45Z

Thanks @joelgriffith for fixing it up, and thanks @DxCx for fixing the travis issues. You guys are awesome!

jadkap · 2017-01-27T16:54:44Z

Thanks all for getting this merged. I was able to try latest and found a couple of things. (BTW, the "schema" issue I previously encountered was a symlink/node_modules issue.) @joelgriffith, if you have time, I'd appreciate your input on #285.

Adding in restify integration, docs, tests more...

d456ff5

Testing failures on PUT calls

77e1f41

DxCx added the tag: new feature label Oct 27, 2016

DxCx added the 🛫 in-progress label Jan 12, 2017

jadkap reviewed Jan 18, 2017

View reviewed changes

DxCx reviewed Jan 19, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/restify-i…

d27b4af

…ntegration

DxCx suggested changes Jan 23, 2017

View reviewed changes

Fixes for Restify packages, test updates, README

30a98ab

DxCx approved these changes Jan 24, 2017

View reviewed changes

DxCx added 3 commits January 24, 2017 12:32

Merge branch 'master' into feature/restify-integration

3d26656

Merge branch 'master' into feature/restify-integration

5b57865

test(restify): fix tests for restify while rebasing

ffe3b98

DxCx added the ready label Jan 24, 2017

DxCx removed the 🛫 in-progress label Jan 24, 2017

Merge branch 'master' into feature/restify-integration

eeb9f1d

helfer merged commit a29d8b6 into apollographql:master Jan 24, 2017

helfer removed the ready label Jan 24, 2017

jadkap mentioned this pull request Jan 26, 2017

Restify: Fix for calling next() #285

Merged

trevor-scheer pushed a commit that referenced this pull request May 6, 2020

chore(deps): update dependency lint-staged to v9.2.4 (#189)

2197c56

trevor-scheer pushed a commit that referenced this pull request May 14, 2020

chore(deps): update dependency lint-staged to v9.2.4 (#189)

4c2aa02

github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding in restify integration, docs, tests more... #189

Adding in restify integration, docs, tests more... #189

joelgriffith commented Oct 24, 2016 •

edited by DxCx

Loading

joelgriffith commented Oct 24, 2016

joelgriffith commented Nov 7, 2016

stubailo commented Nov 7, 2016

helfer commented Nov 29, 2016

helfer commented Dec 7, 2016

jadkap commented Jan 10, 2017

DxCx commented Jan 12, 2017

jadkap commented Jan 17, 2017

DxCx commented Jan 17, 2017

joelgriffith commented Jan 17, 2017 •

edited

Loading

DxCx commented Jan 18, 2017

jadkap left a comment

jadkap Jan 18, 2017

jadkap Jan 18, 2017

DxCx commented Jan 19, 2017

DxCx Jan 19, 2017

joelgriffith commented Jan 19, 2017

DxCx commented Jan 19, 2017

joelgriffith commented Jan 22, 2017

DxCx left a comment

DxCx Jan 23, 2017

DxCx Jan 23, 2017

DxCx Jan 23, 2017

DxCx Jan 23, 2017

DxCx Jan 23, 2017

joelgriffith Jan 23, 2017

joelgriffith commented Jan 23, 2017

DxCx commented Jan 24, 2017

helfer commented Jan 24, 2017

jadkap commented Jan 27, 2017


		server.post('/graphql', graphqlRestify({ schema: myGraphQLSchema }));

		sever.get('/graphiql', graphiqlRestify({ endpointURL: '/graphql' }));


		server.use(restify.bodyParser());

		server.post('/graphql', graphqlRestify({ schema: myGraphQLSchema }));

Adding in restify integration, docs, tests more... #189

Adding in restify integration, docs, tests more... #189

Conversation

joelgriffith commented Oct 24, 2016 • edited by DxCx Loading

joelgriffith commented Oct 24, 2016

joelgriffith commented Nov 7, 2016

stubailo commented Nov 7, 2016

helfer commented Nov 29, 2016

helfer commented Dec 7, 2016

jadkap commented Jan 10, 2017

DxCx commented Jan 12, 2017

jadkap commented Jan 17, 2017

DxCx commented Jan 17, 2017

joelgriffith commented Jan 17, 2017 • edited Loading

DxCx commented Jan 18, 2017

jadkap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DxCx commented Jan 19, 2017

Choose a reason for hiding this comment

joelgriffith commented Jan 19, 2017

DxCx commented Jan 19, 2017

joelgriffith commented Jan 22, 2017

DxCx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelgriffith commented Jan 23, 2017

DxCx commented Jan 24, 2017

helfer commented Jan 24, 2017

jadkap commented Jan 27, 2017

joelgriffith commented Oct 24, 2016 •

edited by DxCx

Loading

joelgriffith commented Jan 17, 2017 •

edited

Loading