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

ApolloServer.logger should probably be public readonly in AS4 #6082

Closed
codedawi opened this issue Feb 4, 2022 · 3 comments
Closed

ApolloServer.logger should probably be public readonly in AS4 #6082

codedawi opened this issue Feb 4, 2022 · 3 comments
Assignees
Milestone

Comments

@codedawi
Copy link

codedawi commented Feb 4, 2022

Proposal

We would like to propose exposing the logger property of ApolloServerBase class that is currently private to any class that extends it by updating it to a protected property.

export class ApolloServerBase<
// The type of the argument to the `context` function for this integration.
ContextFunctionParams = any,
> {
private logger: Logger;

Use case

We have extend the ApolloServerBase class using the serverless approach by implementing a custom handler. We would like to add logging in our handler method but do not want to use console or some global logger. We would like to access the this.logger property configured on initialization of the base class. We do not override constructor function and would prefer not for this a hack around the logger initialization.

export class CustomApolloServerless extends ApolloServerBase {
    protected override serverlessFramework(): boolean {
        return true;
    }
    public createHandler() {
        this.logger.info("Initializing serverless handler...");
        
        return async (request: GraphQLRequest, context: any): Promise<GraphQLResponse> => {
            this.logger.debug(`Handling request ${context.requestId}...`);
        }
    }
}
@glasser
Copy link
Member

glasser commented Feb 4, 2022

Our plan for Apollo Server 4 (see https://github.com/apollographql/apollo-server/blob/main/ROADMAP.md and https://github.com/apollographql/apollo-server/milestone/60) is to change ApolloServer from having a subclass-based API to having a more well-defined public API that you extend by writing separate "createHandler" functions that take the single ApolloServer as an argument. So changing logger to protected definitely won't make sense there. But it might make sense to expose logger as part of ApolloServer's public API, for this exact use case. Right now we're pretty focused on getting AS4 from "started" to "finished" as fast as possible so I'm not sure it makes sense to prioritize fixing this in AS3. I'm going to change this to an issue on the AS4 milestone to consider making logger public.

In the meantime, I don't see any reason we would get rid of ApolloServerBase.logger in AS3, so as a workaround you can write this['logger'], which is a legitimate and type-safe way in TypeScript to access private properties. ie, feel free to consider this as a stable part of the AS3 API even though it is private.

@glasser glasser added this to the Release 4.0 milestone Feb 4, 2022
@glasser glasser changed the title Update logger property from a private to a protected on ApolloServerBase ApolloServer.logger should probably be public in AS4 Feb 4, 2022
@glasser glasser changed the title ApolloServer.logger should probably be public in AS4 ApolloServer.logger should probably be public readonly in AS4 Feb 4, 2022
@glasser
Copy link
Member

glasser commented May 12, 2022

Probably cache too.

@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 in #6668

@glasser glasser closed this as completed Jul 12, 2022
@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