- Notifications
You must be signed in to change notification settings - Fork12.9k
AddverbatimModuleSyntax, deprecateimportsNotUsedAsValues andpreserveValueImports#52203
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
AddverbatimModuleSyntax, deprecateimportsNotUsedAsValues andpreserveValueImports#52203
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
johnnyreilly commentedMar 20, 2023
Okay cool - will do. Was it more trouble than it was worth to maintain this with CommonJS then? |
|
johnnyreilly commentedMar 20, 2023
Interesting. Yeah I always mentally modelled |
xfournet commentedMar 24, 2023
There is no mention of this option inhttps://www.typescriptlang.org/tsconfig (same for all other new options from TS 5.0), is it expected ? |
@xfournet thanks, I openedmicrosoft/TypeScript-Website#2759 |
…rokenNeed to make some decisions about whether/how to emit CJS modules,because as long as we have verbatimModuleSyntax enabled, we cannotuse the typical `export const = "x"` format, and tsc-multi rewritingis also now breaking for this on v5, and patching it to do the rewritesmay be non trivial since it can involve shifting large blocks of code.Alternatively we could just not opt into the flag, in which casewe should also restore the `isolatedModules` flag (which was removedas redunant).See:*https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#verbatimmodulesyntax*microsoft/TypeScript#52203*tommy351/tsc-multi#19
This PR upgrades the Apollo Server repo to use TS 5.x.This change suggests we drop `importsNotUsedAsValues` in favor of thenew[`verbatimModuleSyntax`](https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#verbatimmodulesyntax).However, this rule [isn't really recommended or expected to be adoptedby CommonJSpackages](microsoft/TypeScript#52203 (comment)).So instead of opting in to the new option, I've taken the recommendationin the comment to enforce this via a lint rule which seems to haveaccomplished 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>
…rokenNeed to make some decisions about whether/how to emit CJS modules,because as long as we have verbatimModuleSyntax enabled, we cannotuse the typical `export const = "x"` format, and tsc-multi rewritingis also now breaking for this on v5, and patching it to do the rewritesmay be non trivial since it can involve shifting large blocks of code.Alternatively we could just not opt into the flag, in which casewe should also restore the `isolatedModules` flag (which was removedas redunant).See:*https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#verbatimmodulesyntax*microsoft/TypeScript#52203*tommy351/tsc-multi#19
becaure --importsNotUsedAsValues are being deprecated, more details canbe found atmicrosoft/TypeScript#52203
This PR upgrades the Apollo Server repo to use TS 5.x.This change suggests we drop `importsNotUsedAsValues` in favor of thenew[`verbatimModuleSyntax`](https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#verbatimmodulesyntax).However, this rule [isn't really recommended or expected to be adoptedby CommonJSpackages](microsoft/TypeScript#52203 (comment)).So instead of opting in to the new option, I've taken the recommendationin the comment to enforce this via a lint rule which seems to haveaccomplished 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>
This PR upgrades the Apollo Server repo to use TS 5.x.This change suggests we drop `importsNotUsedAsValues` in favor of thenew[`verbatimModuleSyntax`](https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#verbatimmodulesyntax).However, this rule [isn't really recommended or expected to be adoptedby CommonJSpackages](microsoft/TypeScript#52203 (comment)).So instead of opting in to the new option, I've taken the recommendationin the comment to enforce this via a lint rule which seems to haveaccomplished 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>
Uh oh!
There was an error while loading.Please reload this page.
Closes#51479
This implements pretty much exactly what I proposed in#51479 so I won’t fully re-explain it here. Instead, I’ll call attention to a couple things I didn’t discuss in the proposal.
CommonJS module exports are severely limited
CommonJS JavaScript modules use
requireandmodule.exports =for importing/exporting code. TypeScript has direct equivalents for the basic forms of these:However, TypeScript lacks direct equivalents for destructuring a
requireor doing property assignment exports:It is common to approximate this behavior with ESM syntax in TypeScript:
But this is, in fact, ESM syntax trying to represent a CommonJS module, and is not allowed under
verbatimModuleSyntax. The direct translation is:We knew this would be a bit of a burden for imports, but I didn’t realize what a pain it would be for exports. The problem here is that you can no longer sidecar-export a type:
To accomplish this, you’d have to merge a namespace declaration containing the types:
These limitations make it really unattractive to use the flag in CJS-emitting TypeScript. I discussed this with@DanielRosenwasser, and ultimately decided that it’s not worth worrying about too much—the flag is opt-in, and we expect the users interested in using it to write mostly ESM-emitting TypeScript.
Improved auto-imports for CommonJS imports
Despite the above, I did spend a couple minutes to make auto-imports in CJS files work a bit better. Since
import { writeFile } from "fs"is not allowed in CJS files, when you start typingwriteFile, you now get an auto import offsand the qualification onwriteFileat the same time:🎥 GIF
Pending codefix
I ensured auto-imports work well with the new mode, but I haven’t yet added any codefixes or reviewed the existing ones—I think there are existing ones to add type-only annotations to imports as necessary, but I’ll likely need to add some for transforming imports between CJS and ESM syntaxes. I wanted to get this PR up so it has time to be reviewed before the beta, but I plan to add codefixes so hopefully codebases can be transitioned onto the flag with ts-fix.