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

Apollo Server plugins: expose server instance to plugins #6264

Closed
viglucci opened this issue Mar 26, 2022 · 7 comments
Closed

Apollo Server plugins: expose server instance to plugins #6264

viglucci opened this issue Mar 26, 2022 · 7 comments
Assignees
Milestone

Comments

@viglucci
Copy link

While developing a ApolloServer plugin to control the lifecycle of an RSocket server (in order to support RSocket as a GraphQL transport), I have found that it would be useful to be able to access the ApolloServer instance from within plugins the instance is leveraging.

For instance, using an inline plugin definition I am able to accomplish what I want due the nature of variable scoping, however, this would/could be much cleaner if the serverWillStart(service: GraphQLServiceContext) API included a reference to the ApolloServer instance that the plugin was being used by.

const apolloServer = new RSocketApolloServer({
  typeDefs,
  resolvers,
  plugins: [
    {
      async serverWillStart(service) {
        const requestHandler = apolloServer.getHandler();
        const rsocketServer = new RSocketServer({
          transport: new TcpServerTransport({
            listenOptions: {
              port: 9090,
              host: "127.0.0.1",
            },
          }),
          acceptor: {
            accept: async () => requestHandler,
          },
        });
        let closeable = await rsocketServer.bind();
        return {
          async drainServer() {
            closeable.close();
          },
        };
      },
    },
  ],
});

I feel that this would be much cleaner and a simpler integration pattern if instead written as so:

const rsocketServerOptions = {...};
const apolloServer = new RSocketApolloServer({
  typeDefs,
  resolvers,
  plugins: [
    new RSocketApolloGraphlQLPlugin(rsocketServerOptions),
  ],
});

The later API as shown above would require that the instance of the ApolloServer (RSocketApolloServer in this case) be exposed to the serverWillStart plugin API, so that the instances getHandler() API can be accessed.

@glasser
Copy link
Member

glasser commented Mar 28, 2022

That does sounds like it would be useful. Strangely enough, the current code architecture means that the plugin API is kind of separate from the actual ApolloServer object itself, as a bunch of the code in AS is still structured in a way that's based on Apollo Server 1 where we didn't even have an ApolloServer object. We've cleaned up a lot of that in our version-4 branch so that's probably an appropriate place to consider adding this. Adding to our milestone for consideration.

@glasser glasser added this to the Release 4.0 milestone Mar 28, 2022
@glasser glasser added rebase and removed rebase labels May 12, 2022
glasser added a commit that referenced this issue Jul 12, 2022
…gins

This change does two things, which we originally thought were kind of
connected but maybe aren't.

First, we switch the implementation of contravariance from a hacky
`__forceTContextToBeContravariant` field to the new TS 4.7 variance
annotations. But in fact, instead of making it contravariant, we make it
invariant: `ApolloServer<X>` can only be assigned to `ApolloServer<Y>`
when X and Y are the same. This is because:

- An `ApolloServer<{foo: number}>` is not a `ApolloServer<{}>`, because
  you can invoke `executeOperation` on the latter with an empty context
  object, which would confuse the former (this is the contravariance we
  already had).
- An `ApolloServer<{}>` is not a `ApolloServer<{foo: number}>`, because
  you can invoke `addPlugin` on the latter with a plugin whose methods
  expect to be called with a `contextValue` with `{foo: number}` on it,
  which isn't the case for the former.

Considering the second condition is why this is now invariant (`in
out`).  We're not quite sure why TS couldn't figure this out for us, but
it can't.

Second, we add `server: ApolloServer` to various plugin hook arguments
(`GraphQLRequestContext` and `GraphQLServerContext`). This makes
`logger` and `cache` on `GraphQLRequestContext` somewhat redundant, so
we make those fields into public readonly fields on `ApolloServer` and
remove them from the plugin hook arguments. (We thought this was related
to the other part because adding `server` appeared to require invariance
instead of contravariance, but it turns out that we needed invariance
anyway due to `addPlugin`.)

Note that this means that anyone consuming our `.d.ts` files must be on
TS4.7. Before v4.0.0 we'll probably fix that; see #6423.

Fixes #6264. Fixes #6082.
glasser added a commit that referenced this issue Jul 12, 2022
…gins

This change does two things, which we originally thought were kind of
connected but maybe aren't.

First, we switch the implementation of contravariance from a hacky
`__forceTContextToBeContravariant` field to the new TS 4.7 variance
annotations. But in fact, instead of making it contravariant, we make it
invariant: `ApolloServer<X>` can only be assigned to `ApolloServer<Y>`
when X and Y are the same. This is because:

- An `ApolloServer<{foo: number}>` is not a `ApolloServer<{}>`, because
  you can invoke `executeOperation` on the latter with an empty context
  object, which would confuse the former (this is the contravariance we
  already had).
- An `ApolloServer<{}>` is not a `ApolloServer<{foo: number}>`, because
  you can invoke `addPlugin` on the latter with a plugin whose methods
  expect to be called with a `contextValue` with `{foo: number}` on it,
  which isn't the case for the former.

Considering the second condition is why this is now invariant (`in
out`).  We're not quite sure why TS couldn't figure this out for us, but
it can't.

Second, we add `server: ApolloServer` to various plugin hook arguments
(`GraphQLRequestContext` and `GraphQLServerContext`). This makes
`logger` and `cache` on `GraphQLRequestContext` somewhat redundant, so
we make those fields into public readonly fields on `ApolloServer` and
remove them from the plugin hook arguments. (We thought this was related
to the other part because adding `server` appeared to require invariance
instead of contravariance, but it turns out that we needed invariance
anyway due to `addPlugin`.)

Note that this means that anyone consuming our `.d.ts` files must be on
TS4.7. Before v4.0.0 we'll probably fix that; see #6423.

Fixes #6264. Fixes #6082.
glasser added a commit that referenced this issue Jul 12, 2022
…gins (#6668)

This change does two things, which we originally thought were kind of
connected but maybe aren't.

First, we switch the implementation of contravariance from a hacky
`__forceTContextToBeContravariant` field to the new TS 4.7 variance
annotations. But in fact, instead of making it contravariant, we make it
invariant: `ApolloServer<X>` can only be assigned to `ApolloServer<Y>`
when X and Y are the same. This is because:

- An `ApolloServer<{foo: number}>` is not a `ApolloServer<{}>`, because
  you can invoke `executeOperation` on the latter with an empty context
  object, which would confuse the former (this is the contravariance we
  already had).
- An `ApolloServer<{}>` is not a `ApolloServer<{foo: number}>`, because
  you can invoke `addPlugin` on the latter with a plugin whose methods
  expect to be called with a `contextValue` with `{foo: number}` on it,
  which isn't the case for the former.

Considering the second condition is why this is now invariant (`in
out`).  We're not quite sure why TS couldn't figure this out for us, but
it can't.

Second, we add `server: ApolloServer` to various plugin hook arguments
(`GraphQLRequestContext` and `GraphQLServerContext`). This makes
`logger` and `cache` on `GraphQLRequestContext` somewhat redundant, so
we make those fields into public readonly fields on `ApolloServer` and
remove them from the plugin hook arguments. (We thought this was related
to the other part because adding `server` appeared to require invariance
instead of contravariance, but it turns out that we needed invariance
anyway due to `addPlugin`.)

Note that this means that anyone consuming our `.d.ts` files must be on
TS4.7. Before v4.0.0 we'll probably fix that; see #6423.

Fixes #6264. Fixes #6082.
@glasser
Copy link
Member

glasser commented Jul 12, 2022

Fixed in version-4 by #6668

@glasser
Copy link
Member

glasser commented Aug 15, 2022

We're actually going to undo this planned v4 change. The generic typing of ApolloServerPlugin works a lot smoother if the plugin can only get context values and not send them, and if ApolloServerPlugin is passed an ApolloServer object then it can call server.executeOperation(op, context) which causes the issue. (Basically, it becomes challenging to write a plugin that works with any context value, which is the common case!)

However, we have added an ApolloServer.addPlugin method which can be called after constructing your server but before starting it. So that allows you to pass an ApolloServer directly to your plugin if you'd like. This would allow an API like:

const rsocketServerOptions = {...};
const apolloServer = new RSocketApolloServer({
  typeDefs,
  resolvers,
});
apolloServer.addPlugin(new RSocketApolloGraphlQLPlugin(apolloServer, rsocketServerOptions));

You would want to define RSocketApolloGraphlQLPlugin as

class RSocketApolloGraphlQLPlugin<TContext extends BaseContext> implements ApolloServerPlugin<TContext>

@viglucci
Copy link
Author

Thanks for the update @glasser . I'll experiment with updating our ApolloServer integration to the new recommended pattern next chance I get.

@glasser
Copy link
Member

glasser commented Aug 15, 2022

Actually I guess in AS3 you can also do it like this:

const apolloServer = new RSocketApolloServer({
  typeDefs,
  resolvers,
  plugins: [() => new RSocketApolloGraphlQLPlugin(apolloServer, rsocketServerOptions)],
});

using the plugin factory function feature.

glasser added a commit that referenced this issue Aug 15, 2022
Previously on `version-4`, we had made `ApolloServerPlugin<TContext>`
"invariant" in `TContext` via the new `in out` annotation. That's
because we had added a `server` field to `GraphQLRequestContext` and
`GraphQLServerContext`.

The issue here is that because you can invoke
`server.executeOperation(op, contextValue)`, then a
`ApolloServerPlugin<BaseContext>` was not assignable to
`ApolloServerPlugin<SomeSpecificContext>`, because the former was able
to call `requestContext.executeOperation(op, {})`, and a server whose
context has additional required fields could not load this plugin. This
essentially meant that you couldn't have a single object (not created by
a generic function or generic class) that was a plugin that works with
any server, even if it doesn't actually care about context values at
all.

When trying to dogfood AS4 it became clear that this was annoying. So
let's fix it by removing `server` from those objects and making
`ApolloServerPlugin` "contravariant" instead. This means that plugins
can only *receive* `contextValue`, not *send* it. And so
`ApolloServerPlugin<BaseContext>` (a plugin that doesn't bother to read
any fields from `contextValue`) can be assigned to
`ApolloServerPlugin<SpecificContext>` (a plugin that is permitted to
read specific fields on `contextValue`, though it may choose not to).

After removing `server`, restore `logger` (now `readonly`) and `cache`
to `GraphQLRequestContext` and `GraphQLServerContext` (`cache` is
actually new to `GraphQLServerContext` in AS4). They had previously been
removed for redundancy.

This un-does the fix to #6264. If your plugin needs access to the
`server` object, you can just pass it to your plugin's
constructor/factory yourself. This wasn't an option in AS3 because there
was no `server.addPlugin` method... or actually, you could do it with
the plugin factory feature.

Note that we do leave `ApolloServer<TContext>` as invariant in
`TContext`, for the reasons described in a comment above the class.

While we're at it, remove the unnecessary ability to specify plugins as
factory functions. As far as we know, the main use case for this feature
is to refer to the `ApolloServer` itself in the expression creating the
plugin, but the new `addPlugin` method lets you do the same thing.
Reducing the number of ways to specify constructor options helps keep
type errors simpler.
glasser added a commit that referenced this issue Aug 15, 2022
Previously on `version-4`, we had made `ApolloServerPlugin<TContext>`
"invariant" in `TContext` via the new `in out` annotation. That's
because we had added a `server` field to `GraphQLRequestContext` and
`GraphQLServerContext`.

The issue here is that because you can invoke
`server.executeOperation(op, contextValue)`, then a
`ApolloServerPlugin<BaseContext>` was not assignable to
`ApolloServerPlugin<SomeSpecificContext>`, because the former was able
to call `requestContext.executeOperation(op, {})`, and a server whose
context has additional required fields could not load this plugin. This
essentially meant that you couldn't have a single object (not created by
a generic function or generic class) that was a plugin that works with
any server, even if it doesn't actually care about context values at
all.

When trying to dogfood AS4 it became clear that this was annoying. So
let's fix it by removing `server` from those objects and making
`ApolloServerPlugin` "contravariant" instead. This means that plugins
can only *receive* `contextValue`, not *send* it. And so
`ApolloServerPlugin<BaseContext>` (a plugin that doesn't bother to read
any fields from `contextValue`) can be assigned to
`ApolloServerPlugin<SpecificContext>` (a plugin that is permitted to
read specific fields on `contextValue`, though it may choose not to).

After removing `server`, restore `logger` (now `readonly`) and `cache`
to `GraphQLRequestContext` and `GraphQLServerContext` (`cache` is
actually new to `GraphQLServerContext` in AS4). They had previously been
removed for redundancy.

This un-does the fix to #6264. If your plugin needs access to the
`server` object, you can just pass it to your plugin's
constructor/factory yourself. This wasn't an option in AS3 because there
was no `server.addPlugin` method... or actually, you could do it with
the plugin factory feature.

Note that we do leave `ApolloServer<TContext>` as invariant in
`TContext`, for the reasons described in a comment above the class.

While we're at it, remove the unnecessary ability to specify plugins as
factory functions. As far as we know, the main use case for this feature
is to refer to the `ApolloServer` itself in the expression creating the
plugin, but the new `addPlugin` method lets you do the same thing.
Reducing the number of ways to specify constructor options helps keep
type errors simpler.
glasser added a commit that referenced this issue Aug 15, 2022
…6814)

Previously on `version-4`, we had made `ApolloServerPlugin<TContext>`
"invariant" in `TContext` via the new `in out` annotation. That's
because we had added a `server` field to `GraphQLRequestContext` and
`GraphQLServerContext`.

The issue here is that because you can invoke
`server.executeOperation(op, contextValue)`, then a
`ApolloServerPlugin<BaseContext>` was not assignable to
`ApolloServerPlugin<SomeSpecificContext>`, because the former was able
to call `requestContext.executeOperation(op, {})`, and a server whose
context has additional required fields could not load this plugin. This
essentially meant that you couldn't have a single object (not created by
a generic function or generic class) that was a plugin that works with
any server, even if it doesn't actually care about context values at
all.

When trying to dogfood AS4 it became clear that this was annoying. So
let's fix it by removing `server` from those objects and making
`ApolloServerPlugin` "contravariant" instead. This means that plugins
can only *receive* `contextValue`, not *send* it. And so
`ApolloServerPlugin<BaseContext>` (a plugin that doesn't bother to read
any fields from `contextValue`) can be assigned to
`ApolloServerPlugin<SpecificContext>` (a plugin that is permitted to
read specific fields on `contextValue`, though it may choose not to).

After removing `server`, restore `logger` (now `readonly`) and `cache`
to `GraphQLRequestContext` and `GraphQLServerContext` (`cache` is
actually new to `GraphQLServerContext` in AS4). They had previously been
removed for redundancy.

This un-does the fix to #6264. If your plugin needs access to the
`server` object, you can just pass it to your plugin's
constructor/factory yourself. This wasn't an option in AS3 because there
was no `server.addPlugin` method... or actually, you could do it with
the plugin factory feature.

Note that we do leave `ApolloServer<TContext>` as invariant in
`TContext`, for the reasons described in a comment above the class.

While we're at it, remove the unnecessary ability to specify plugins as
factory functions. As far as we know, the main use case for this feature
is to refer to the `ApolloServer` itself in the expression creating the
plugin, but the new `addPlugin` method lets you do the same thing.
Reducing the number of ways to specify constructor options helps keep
type errors simpler.
@viglucci
Copy link
Author

viglucci commented Sep 4, 2022

@glasser I'm finally getting around to experimenting with your suggestion. For V3, looks like I'll also need to add a setApolloServer option as referencing the server variable throws a reference error with your suggested approach. Maybe this is just a TypeScript constraint?

  const server = new RSocketApolloServer({
    typeDefs,
    resolvers,
    plugins: [
      () =>
        new RSocketApolloGraphlQLPlugin({
          apolloServer: server,
          makeRSocketServer,
        }),
    ],
  });
ReferenceError: Cannot access 'server' before initialization

Either way, I think this works sufficiently well with a setApolloServer helper. Thanks again for the recommended workarounds.

  const plugin = new RSocketApolloGraphlQLPlugin({ makeRSocketServer });
  const server = new RSocketApolloServer({
    typeDefs,
    resolvers,
    plugins: [() => plugin],
  });
  plugin.setApolloServer(server);

@glasser
Copy link
Member

glasser commented Sep 8, 2022

You're right.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants