mickhansen / dataloader-sequelize

Batching and simplification of Sequelize with facebook/dataloader
MIT License
284 stars 58 forks source link

Usage without graphql-sequelize #115

Closed bwoodlt closed 4 years ago

bwoodlt commented 4 years ago

Thanks for this library!

I haven't been able to use it properly as I'm not using graphql-sequelize resolvers. Perhaps I'm missing something very obvious...

My current setup is as follows:

const apolloServer = new ApolloServer({
  schema,
   context: ({ req, res }) => {
    const loaderContext = createContext(db);
    const dataloaderContext = { [EXPECTED_OPTIONS_KEY]: loaderContext };
    return {
      redis,
      models: db.models,
      user: req && req.user ? req.user : null,
      pubsub,
      dataloaderContext,
    };
  },
})

// Then in my getter, I have this 
const userInfo = await db.models.users.findByPk(args.id);
// this worked but it was really slow compared to when using findOne({...})

I have a good feeling im not using this correctly. I'd appreciate any help.

intellix commented 4 years ago

You need to pass the dataloader to your find calls:

User.findByPk(1, { [EXPECTED_OPTIONS_KEY]: loaderContext });
bwoodlt commented 4 years ago

@intellix yes I did try that...

I think I followed your approach in another github issue :)

const userInfo = await db.models.users.findByPk(args.id, dataLoaderContext)

I assume the above is equivalent to what you meant?

mickhansen commented 4 years ago

@bwoodlt the dataloader context needs to be a part of the options object passed to a call, not the entire object. @intellix's sample is correct

bwoodlt commented 4 years ago

@mickhansen @intellix Thanks for the quick turnaround!

Following your feedback, I've modified to the following:

  context: ({ req, res }) => {
    const loaderContext = createContext(db);

    return {
      redis,
      url: req ? `${req.protocol}://${req.get("hosts")}` : "",
      req,
      res,
      models: db.models,
      user: req && req.user ? req.user : null,
      pubsub,
      loaderContext,
    };
  },

// Usage
    user: {
      type: UserType,
      args: {
        id: {
          type: GraphQLInt,
        }
      },
      resolve: isAuthenticated.createResolver(async (roots, args, { user, loaderContext }) => {
        if (!user) throw new GraphQLError("Unauthorised User!!");
        try {
          return await db.models.users.findByPk(args.id, { [EXPECTED_OPTIONS_KEY]: loaderContext });
        } catch (error) {
          return new GraphQLError(error);
        }
      }),
    },

I find that the call to findByPk is a little slower.

My sincere appreciation!

mickhansen commented 4 years ago

@bwoodlt That should be enough yes. For simplicity you could add EXPECTED_OPTIONS_KEY to the graphql context and simply pass the context as options (also lets you set logging and transactions from the context if need be).

As for optimising findAll, there's no inherint way to do it, the library makes no attempt to find queries with similiar where statements and merge them, so you'd want to write your own custom dataloaders.

bwoodlt commented 4 years ago

Thanks alot! I'll close this now.