Improve error message for users of old TypeScript versions #6423
Milestone
Comments
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
added a commit
that referenced
this issue
Jul 22, 2022
This is an attempt at addressing #6423. However, it sure seems like `typesVersions` is kinda janky and doesn't work as well as we'd like. One particular thing I've found is that it doesn't play well with the `../dist` lines we have in our package.json files, because it only gets applied if the previously resolved path is under the directory it is in: https://github.com/microsoft/TypeScript/blob/7b764164ede080afff02ec731dfea72b3f4f73f3/src/compiler/moduleNameResolver.ts#L1942 (Yeah, the typesVersions clauses are not consistent across all of the files here: I was doing a test that only looked at the top-level and `/standalone` ones so those are the ones I was iterating on.) It's possible this is fixable but maybe we should just bump the TS requirement to v4.7.
|
This might not be possible; see #6716. We may just need to bump the requirement to v4.7. For now, adding a note requiring v4.7 to the migration guide, but let's reconsider this before the final release (and make sure other parts of the docs show the required version of TS if we do need it to be 4.70. |
|
@glasser Something that can help is making an explicit error for everyone trying to use older TS versions. |
|
That's a good idea. Unfortunately it looks like it depends on the typesVersions feature which is what I had trouble getting to work here... but maybe getting it work in an error-y way is easier than getting it to actually work. |
|
@glasser It should work, since it use an asterisk for the happy path, it shouldn't change TS behavior: |
glasser
changed the title
|
post-RC OK |
|
I tried getting this to work with our setup for a bit and didn't make too much progress. TS4.7 has been out for 4+ months at this point anyway. The sort of error that old TS users will get is: Hopefully folks searching for it will find this issue and discover that they need to upgrade to TS4.7 (which is also in the migration guide). |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
We intend to use some brand-new TS 4.7 features (variance annotations) so let's add https://github.com/sandersn/downlevel-dts to the build process so we can have dts files that work with older TypeScript. We can set up the build script now rather than waiting for 4.7 to be out.
The text was updated successfully, but these errors were encountered: