Upgrade to TypeScript 5.x #7454

trevor-scheer · 2023-03-17T15:48:10Z

This PR upgrades the Apollo Server repo to use TS 5.x.

This change suggests we drop importsNotUsedAsValues in favor of the new verbatimModuleSyntax.

However, this rule isn't really recommended or expected to be adopted by CommonJS packages. So instead of opting in to the new option, I've taken the recommendation in the comment to enforce this via a lint rule which seems to have accomplished the thing we hoped to get out of importsNotUsedAsValues in the first place (but better, for some reason?).

netlify · 2023-03-17T15:48:18Z

✅ Deploy Preview for apollo-server-docs ready!

Name	Link
🔨 Latest commit	`c05fe1c`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-server-docs/deploys/642464b6ac3f5200086e1c68
😎 Deploy Preview	https://deploy-preview-7454--apollo-server-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

codesandbox-ci · 2023-03-17T15:48:54Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c05fe1c:

Sandbox	Source
Apollo Server Typescript	Configuration
Apollo Server	Configuration

glasser

Sounds good as long as we aren't accidentally changing the CJS build to require unnecessary files. (And if the generated DTS doesn't have new syntax in it that would break TS4 users — can you maybe do a manual smoke test that the built package can be loaded in TS4?)

Taking a glance at the codesandbox build... I see that the top-level export * from externalTypes in cjs/index.js does end up sticking around as a require, but the actual cjs/externalTypes/index.js is tiny (so I guess that's a special-casing of export *). ie, loading the package doesn't load the protobuf package just because some of its types are used in externalTypes.

glasser · 2023-03-17T19:39:41Z

tsconfig.build.cjs.json

@@ -1,6 +1,7 @@
 {
  "compilerOptions": {
-    "composite": true
+    "composite": true,
+    "verbatimModuleSyntax": false,


does tsconfig.json support comments (I know it supports trailing commas so it's not pure JSON)? Might be worth calling this one out.

glasser · 2023-03-17T19:40:30Z

tsconfig.build.esm.json

@@ -1,6 +1,7 @@
 {
  "compilerOptions": {
-    "composite": true
+    "composite": true,
+    "verbatimModuleSyntax": true,


curious why this only goes in .build. when the old one also covered tests

glasser · 2023-03-17T19:41:29Z

tsconfig.base.json

@@ -17,7 +17,6 @@
    "noUnusedParameters": true,
    "noUnusedLocals": true,
    "forceConsistentCasingInFileNames": true,
-    "importsNotUsedAsValues": "error",


Do we want to turn this back on in the cjs file?

This is only an error thing that we're changing, so we don't think that turning this off will add unecessary requires calls in the CJS build right?

glasser · 2023-03-17T19:42:20Z

.changeset/late-garlics-sneeze.md

@@ -0,0 +1,2 @@
+---


"we now build with TS5, we don't think it affects callers though" might be reasonable just in case it does break things!

trevor-scheer · 2023-03-28T00:51:06Z

Duping the comment from the datasource-rest PR:

@glasser after some further reading, it looks like we should just drop this option and not opt-in to the new verbatimModuleSyntax until we're building for ESM only. The recommendation from the author of the new option seems to be: CommonJS projects probably don't want this, and can achieve the old thing with proper linting.
microsoft/TypeScript#52203 (comment)

Based on the results of turning on this lint option, it looks like importsNotUsedAsValues didn't quite do what we thought it did? Though to be honest I don't understand why that's the case. Some comments from the mentioned PR^ try to explain it, but my understanding from the docs is that it should've been equivalent to this lint rule.

glasser

I suppose switching away from a deprecated thing is a good idea even if we can't use its real replacement. I do feel like I catch things faster when TSC complains than when ESLint complains, but it's all the same in the end.

@trevor-scheer

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/server@4.6.0 ### Minor Changes - [#7465](#7465) [`1e808146a`](1e80814) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Introduce new opt-in configuration option to mitigate v4 status code regression Apollo Server v4 accidentally started responding to requests with an invalid `variables` object with a 200 status code, where v3 previously responded with a 400. In order to not break current behavior (potentially breaking users who have creatively worked around this issue) and offer a mitigation, we've added the following configuration option which we recommend for all users. ```ts new ApolloServer({ // ... status400ForVariableCoercionErrors: true, }); ``` Specifically, this regression affects cases where _input variable coercion_ fails. Variables of an incorrect type (i.e. `String` instead of `Int`) or unexpectedly `null` are examples that fail variable coercion. Additionally, missing or incorrect fields on input objects as well as custom scalars that throw during validation will also fail variable coercion. For more specifics on variable coercion, see the "Input Coercion" sections in the [GraphQL spec](https://spec.graphql.org/June2018/#sec-Scalars). This will become the default behavior in Apollo Server v5 and the configuration option will be ignored / no longer needed. ### Patch Changes - [#7454](#7454) [`f6e3ae021`](f6e3ae0) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start building packages with TS 5.x, which should have no effect for users - [#7433](#7433) [`e0db95b96`](e0db95b) Thanks [@KGAdamCook](https://github.com/KGAdamCook)! - Previously, when users provided their own `documentStore`, Apollo Server used a random prefix per schema in order to guarantee there was no shared state from one schema to the next. Now Apollo Server uses a hash of the schema, which enables the provided document store to be shared if you choose to do so. ## @apollo/server-integration-testsuite@4.6.0 ### Patch Changes - [#7454](#7454) [`f6e3ae021`](f6e3ae0) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start building packages with TS 5.x, which should have no effect for users - Updated dependencies \[[`1e808146a`](1e80814), [`f6e3ae021`](f6e3ae0), [`e0db95b96`](e0db95b)]: - @apollo/server@4.6.0 ## @apollo/server-plugin-response-cache@4.1.2 ### Patch Changes - [#7454](#7454) [`f6e3ae021`](f6e3ae0) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start building packages with TS 5.x, which should have no effect for users Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

This PR upgrades the Apollo Server repo to use TS 5.x. This change suggests we drop `importsNotUsedAsValues` in favor of the new [`verbatimModuleSyntax`](https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#verbatimmodulesyntax). However, this rule [isn't really recommended or expected to be adopted by CommonJS packages](microsoft/TypeScript#52203 (comment)). So instead of opting in to the new option, I've taken the recommendation in the comment to enforce this via a lint rule which seems to have accomplished the thing we hoped to get out of `importsNotUsedAsValues` in the first place (but better, for some reason?). --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

@trevor-scheer

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/server@4.6.0 ### Minor Changes - [#7465](#7465) [`1e808146a`](1e80814) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Introduce new opt-in configuration option to mitigate v4 status code regression Apollo Server v4 accidentally started responding to requests with an invalid `variables` object with a 200 status code, where v3 previously responded with a 400. In order to not break current behavior (potentially breaking users who have creatively worked around this issue) and offer a mitigation, we've added the following configuration option which we recommend for all users. ```ts new ApolloServer({ // ... status400ForVariableCoercionErrors: true, }); ``` Specifically, this regression affects cases where _input variable coercion_ fails. Variables of an incorrect type (i.e. `String` instead of `Int`) or unexpectedly `null` are examples that fail variable coercion. Additionally, missing or incorrect fields on input objects as well as custom scalars that throw during validation will also fail variable coercion. For more specifics on variable coercion, see the "Input Coercion" sections in the [GraphQL spec](https://spec.graphql.org/June2018/#sec-Scalars). This will become the default behavior in Apollo Server v5 and the configuration option will be ignored / no longer needed. ### Patch Changes - [#7454](#7454) [`f6e3ae021`](f6e3ae0) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start building packages with TS 5.x, which should have no effect for users - [#7433](#7433) [`e0db95b96`](e0db95b) Thanks [@KGAdamCook](https://github.com/KGAdamCook)! - Previously, when users provided their own `documentStore`, Apollo Server used a random prefix per schema in order to guarantee there was no shared state from one schema to the next. Now Apollo Server uses a hash of the schema, which enables the provided document store to be shared if you choose to do so. ## @apollo/server-integration-testsuite@4.6.0 ### Patch Changes - [#7454](#7454) [`f6e3ae021`](f6e3ae0) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start building packages with TS 5.x, which should have no effect for users - Updated dependencies \[[`1e808146a`](1e80814), [`f6e3ae021`](f6e3ae0), [`e0db95b96`](e0db95b)]: - @apollo/server@4.6.0 ## @apollo/server-plugin-response-cache@4.1.2 ### Patch Changes - [#7454](#7454) [`f6e3ae021`](f6e3ae0) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start building packages with TS 5.x, which should have no effect for users Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

This PR upgrades the Apollo Server repo to use TS 5.x. This change suggests we drop `importsNotUsedAsValues` in favor of the new [`verbatimModuleSyntax`](https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#verbatimmodulesyntax). However, this rule [isn't really recommended or expected to be adopted by CommonJS packages](microsoft/TypeScript#52203 (comment)). So instead of opting in to the new option, I've taken the recommendation in the comment to enforce this via a lint rule which seems to have accomplished the thing we hoped to get out of `importsNotUsedAsValues` in the first place (but better, for some reason?). --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

chore(deps): update dependency typescript to v5

cfa1c89

trevor-scheer requested a review from glasser March 17, 2023 15:48

glasser reviewed Mar 17, 2023

View reviewed changes

Enforce type imports via linting

9b77011

trevor-scheer force-pushed the trevor/ts-5.x branch from 0929806 to 9b77011 Compare March 27, 2023 22:30

changeset

6a7d13c

trevor-scheer marked this pull request as ready for review March 28, 2023 00:51

fix style

c05fe1c

glasser approved these changes Mar 29, 2023

View reviewed changes

trevor-scheer merged commit f6e3ae0 into main Mar 29, 2023
6 checks passed

trevor-scheer deleted the trevor/ts-5.x branch March 29, 2023 17:57

github-actions bot mentioned this pull request Mar 29, 2023

Version Packages #7473

Merged

github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to TypeScript 5.x #7454

Upgrade to TypeScript 5.x #7454

trevor-scheer commented Mar 17, 2023 •

edited

netlify bot commented Mar 17, 2023 •

edited

codesandbox-ci bot commented Mar 17, 2023 •

edited

glasser left a comment

glasser Mar 17, 2023

glasser Mar 17, 2023

glasser Mar 17, 2023

glasser Mar 17, 2023

trevor-scheer commented Mar 28, 2023

glasser left a comment

Upgrade to TypeScript 5.x #7454

Upgrade to TypeScript 5.x #7454

Conversation

trevor-scheer commented Mar 17, 2023 • edited

netlify bot commented Mar 17, 2023 • edited

✅ Deploy Preview for apollo-server-docs ready!

codesandbox-ci bot commented Mar 17, 2023 • edited

glasser left a comment

Choose a reason for hiding this comment

glasser Mar 17, 2023

Choose a reason for hiding this comment

glasser Mar 17, 2023

Choose a reason for hiding this comment

glasser Mar 17, 2023

Choose a reason for hiding this comment

glasser Mar 17, 2023

Choose a reason for hiding this comment

trevor-scheer commented Mar 28, 2023

glasser left a comment

Choose a reason for hiding this comment

trevor-scheer commented Mar 17, 2023 •

edited

netlify bot commented Mar 17, 2023 •

edited

codesandbox-ci bot commented Mar 17, 2023 •

edited