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

[Draft] Transition to graphql-modules v1 #1148

Conversation

darkbasic
Copy link
Member

This is just a draft and the API is subject to changes.
It's already working, but there are many changes I want to do.

@changeset-bot
Copy link

changeset-bot bot commented Jun 9, 2021

⚠️ No Changeset found

Latest commit: 44ae64a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@darkbasic
Copy link
Member Author

darkbasic commented Jun 10, 2021

@pradel unfortunately graphql-modules v1 doesn't play well with apollo-server: Urigo/graphql-modules#1725
You're basically forced to use graphql-modules in your project and even then your latencies will double (Urigo/graphql-modules#1724).

The fix would be easy but it's passed more than 1 year since the PR has been created and still no signs of merging: apollographql/apollo-server#4167

@darkbasic
Copy link
Member Author

For the moment I simply updated the graphql-server-typescript example to simply use graphql-modules itself.

@pradel
Copy link
Member

pradel commented Jun 10, 2021

The latency issue is really bad... It's pretty hard to justify the upgrade for the users if the execution is 2X slower

@pradel
Copy link
Member

pradel commented Jun 10, 2021

So there is no way to not use graphql-modules by just using the schema it provides like it was possible with v0?

@darkbasic
Copy link
Member Author

Not with Apollo server, but it's possible with other GraphQL servers.

@pradel
Copy link
Member

pradel commented Jul 25, 2021

@darkbasic any update here? I saw that the guild released a new package https://the-guild.dev/blog/introducing-envelop could it be something that we could use here?

@darkbasic
Copy link
Member Author

AFAIK envelop is not fully supported on Apollo as well. I will have to think more about this, the problem is Apollo not supporting custom execute.

@pradel
Copy link
Member

pradel commented Jul 26, 2021

Ok so no good solutions for now.. Wondering if it makes sense to finish/release the other graphql-modules v1 migration to unblock some users.

@darkbasic
Copy link
Member Author

You mean the other PR? It faces the same Apollo limitations.

@pradel
Copy link
Member

pradel commented Jul 26, 2021

Yeah it does face the same apollo limitations but at the same time it's working, just performance is really bad (This should be in the accounts-js docs to warn users)... I know some people are blocked by graphql-modules v0 and really need the migration to v1. You can also use the v1 without apollo by using other graphql servers.

@darkbasic
Copy link
Member Author

Yeah it does face the same apollo limitations but at the same time it's working

Can you please tell me which PR do you refer to? As far as I remember they both weren't working as well.

@pradel
Copy link
Member

pradel commented Jul 27, 2021

Ah okay then maybe I misunderstood something, I am talking about this one #1141

@pradel
Copy link
Member

pradel commented Jul 27, 2021

I also just saw that apollo released apollo server v3, don't know if it's fixed in this version.

@darkbasic
Copy link
Member Author

Nope, they just ignored my request: apollographql/apollo-server#5262 (comment)

Feel free to ping them about it.

@darkbasic
Copy link
Member Author

A workaround would be using something like this: Urigo/graphql-modules#1724 (comment)

@pradel
Copy link
Member

pradel commented Jul 29, 2021

Yeah definitely not perfect but at least it would work..

@darkbasic
Copy link
Member Author

I will try to give it another shot next week to refresh my mind about the possible alternatives.

@darkbasic
Copy link
Member Author

@pradel it took more than I anticipated to find some spare time to give this another try, but today I've managed to find some and this is how it's going to look like with apollo-server and the new API:

import { createApplication } from 'graphql-modules';

const server = new ApolloServer({
  schema: createApplication({
    modules: [
      createAccountsCoreModule({ accountsServer }),
      createAccountsPasswordModule({ accountsPassword }),
    ],
    schemaBuilder: ({ typeDefs: accountsTypeDefs, resolvers: accountsResolvers }) =>
      makeExecutableSchema({
        typeDefs: mergeTypeDefs([typeDefs, ...accountsTypeDefs]),
        resolvers: mergeResolvers([...accountsResolvers, resolvers]),
      }),
  }).createSchemaForApollo(),
  context: ({ req, connection }) => {
    return context({ req, connection }, { accountsServer });
  },
});

server.listen(4000).then(({ url }) => {
  console.log(`🚀  Server ready at ${url}`);
});

and this is with boosts:

const accounts = await accountsBoost({
  services: {
    password: {
        [...]
      },
    },
  },
  tokenSecret: 'secret',
  schemaBuilder: ({ typeDefs: accountsTypeDefs, resolvers: accountsResolvers }) =>
    makeExecutableSchema({
      typeDefs: mergeTypeDefs([typeDefs, ...accountsTypeDefs]),
      resolvers: mergeResolvers([...accountsResolvers, resolvers]),
    }),
} as any);

accounts.listen({
  port: 4000,
});

If this is ok I'll rebase my branch (or rewrite it from scratch since I also need to port the codebase to the latest graphql-tools).

@pradel
Copy link
Member

pradel commented Oct 10, 2021

@darkbasic Looks great!

@darkbasic
Copy link
Member Author

I've finished re-porting from scratch latest master to GraphQL Modules V1 and I've also updated every single dep to latest because I was annoyed by the tons of warnings.
Now it starts the long, tedious process of re-porting all of the examples (and testing that everything still works).
It probably makes sense to open the new PR only once I've finished with the examples, then you can start testing while I will port my WIP MikroORM adapter to GraphQL Modules V1 and mikro-orm v5.
At that point I will be able to know if the new API fits my needs (and thus I can port my application to accounts-js) or if I will have to make further improvements.

@pradel
Copy link
Member

pradel commented Oct 16, 2021

Amazing work! Can't wait to merge the pr :D

@darkbasic
Copy link
Member Author

Sorry for taking that long, but I've rewrote the schema stitching example from scratch and I've found a bug in GraphQL Modules which I took quite some time to figure out. I've managed to fix it, but it will require a new GraphQL Modules release. I'll keep you posted.

@darkbasic
Copy link
Member Author

There are still a few things I need to test before I will open the new PR, but in the meantime if you want you can have a look at https://github.com/darkbasic/accounts/tree/v1

@darkbasic
Copy link
Member Author

PR is ready (#1189), I'm closing this.

@darkbasic darkbasic closed this Oct 25, 2021
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