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(server): add serverless handlers #788

Merged
merged 20 commits into from May 6, 2020
Merged

Conversation

jasonkuhrt
Copy link
Member

@jasonkuhrt jasonkuhrt commented May 3, 2020

progresses #782
closes #528

Implementation Notes

Several deps have been added in this PR. The goal is to move quickly and iterate on our server components. None of the deps are large, even fp-ts.

fp-ts is exciting. It begins our journey into throw-free code!

User Notes

This feature will remain undocumented while we make progress on more parts around it.

This feature exposes request handlers on the server component.

import { server } from 'nexus'

server.handlers.graphql
server.handlers.playground

Users can use these to response to requests in a serverless environment.

import { server } from 'nexus'

export default (req, res) => {
  server.handlers.graphql(req, res)
}

TODO

  • docs
  • tests

@jasonkuhrt jasonkuhrt changed the title feat(server): add serverless endpoints feat(server): add serverless handlers May 4, 2020
@jasonkuhrt jasonkuhrt requested a review from Weakky May 5, 2020 14:38
@jasonkuhrt jasonkuhrt marked this pull request as ready for review May 5, 2020 19:14
@jasonkuhrt
Copy link
Member Author

image

"dotenv": "^8.2.0",
"express": "^4.17.1",
"express-graphql": "^0.9.0",
"fp-ts": "^2.5.4",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced by the need of something as fancy as fp-ts yet. As a first step, we could've just created our own internal either type, or either-like type. While I see great patterns that we could achieve with fp-ts, we need to make sure that its types don't ever land on the public API, because even something as popular as the Either type in the fp world is not common at all in the JS world

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we’ll definitely not want the FP API/types escaping the implementation. It might mean we need to eventually maintain a thin layer on top. Really whatever helps us the most... the type safety added by not having untyped throw is great for us internally for example. Add more and more benefits like this and I could see reaching a point where net velocity/simplicity for us is had by adopting the fp implementation with a thin non-FP layer.

Internally I think it’s worth using the gold standard lib to properly appraise the techniques. It’s not like we’re adding many mb or kb to the nexus package. And the path forward to more FP features that rigorously compose is available when we’re ready. Like rx and lodash it is free shakable so once we care we can look into that.

assemble() {
if (appState.assembled) return

appState.assembled = true
appState.assembled = {} as any
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you use a Partial here?

Copy link
Member Author

@jasonkuhrt jasonkuhrt May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a todo for now

image

Think the settings readonly is causing issue. Tried Mutable modifier from type fest (also related: microsoft/TypeScript#24509) on the assembled app state settings type, but didn't seem to help.

Comment on lines 33 to 34
// schema: null,
// missingTypes: null,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the comments or are they supposed to come back later?

Copy link
Collaborator

@Weakky Weakky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all great to me 🚀, great job 🙌

@jasonkuhrt jasonkuhrt merged commit 7cfd27c into master May 6, 2020
@jasonkuhrt jasonkuhrt deleted the feat/serverless-endpoints branch May 6, 2020 11:29
@@ -117,7 +119,8 @@ export function create(): App {
assemble() {
if (appState.assembled) return

appState.assembled = {} as any
// todo https://github.com/graphql-nexus/nexus/pull/788#discussion_r420645846
appState.assembled = {} as Partial<AppState['assembled']>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tweak made all tests failed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this wasn't supposed to get through :(

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.

Remove express-graphql
2 participants