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

Add verbatimModuleSyntax, deprecate importsNotUsedAsValues and preserveValueImports #52203

Merged
merged 19 commits into from
Jan 20, 2023

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Jan 12, 2023

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 require and module.exports = for importing/exporting code. TypeScript has direct equivalents for the basic forms of these:

// JS
const fs = require("fs");
module.exports = {};
// TS
import fs = require("fs");
export = {};

However, TypeScript lacks direct equivalents for destructuring a require or doing property assignment exports:

// JS
const { writeFile } = require("fs");
module.exports.hello = "world";

It is common to approximate this behavior with ESM syntax in TypeScript:

import { writeFile } from "fs";
export const hello = "world";

But this is, in fact, ESM syntax trying to represent a CommonJS module, and is not allowed under verbatimModuleSyntax. The direct translation is:

import fs = require("fs");
// must use `fs.writeFile` or assign to a variable
export = { hello: "world" };

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:

export type T = {};
export = { hello: "world" };
// ^^^^^^^^^^^^^^^^^^^^^^^^
// An export assignment cannot be used in a module with other exported elements.(2309)

To accomplish this, you’d have to merge a namespace declaration containing the types:

const _exports = { hello: "world" };
namespace _exports {
  export type T = {};
}
export = _exports;

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 typing writeFile, you now get an auto import of fs and the qualification on writeFile at the same time:

🎥 GIF

Kapture 2023-01-11 at 16 31 39

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.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jan 12, 2023
@andrewbranch andrewbranch marked this pull request as ready for review January 12, 2023 01:06
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
@andrewbranch
Copy link
Member Author

That’s right; you’ll want to use typescript-eslint’s consistent-type-imports, IIRC.

@johnnyreilly
Copy link

Okay cool - will do. Was it more trouble than it was worth to maintain this with CommonJS then?

@andrewbranch
Copy link
Member Author

importsNotUsedAsValues was made to serve the opposite purpose you (and basically everyone) were using it for. By default, TypeScript elides unneeded import statements from JS emit even without marking them as type imports. importsNotUsedAsValues was created as a way to escape that behavior, not (primarily) to make it more explicit. verbatimModuleSyntax allows you to escape the elision behavior, and takes the explicitness of what your imports mean a step further by making you write CJS-style imports when emitting to CJS. So in my book, all the important cases that importsNotUsedAsValues (and preserveValueImports) covered, plus more, are covered by verbatimModuleSyntax, which is way more explainable. It’s mostly a matter of reducing complexity for the sake of explanation.

@johnnyreilly
Copy link

Interesting. Yeah I always mentally modelled "importsNotUsedAsValues": "error" as "strictTypeImports": true - essentially it was making code more explicit for me. I guess this is a linting concern rather than a compilation concern - I shall make use of https://typescript-eslint.io/rules/consistent-type-imports/

@xfournet
Copy link

There is no mention of this option in https://www.typescriptlang.org/tsconfig (same for all other new options from TS 5.0), is it expected ?

@andrewbranch
Copy link
Member Author

@xfournet thanks, I opened microsoft/TypeScript-Website#2759

@PolariTOON PolariTOON mentioned this pull request Mar 28, 2023
5 tasks
milesrichardson added a commit to splitgraph/madatdata that referenced this pull request Mar 28, 2023
…roken

Need to make some decisions about whether/how to emit CJS modules,
because as long as we have verbatimModuleSyntax enabled, we cannot
use the typical `export const = "x"` format, and tsc-multi rewriting
is also now breaking for this on v5, and patching it to do the rewrites
may be non trivial since it can involve shifting large blocks of code.

Alternatively we could just not opt into the flag, in which case
we should also restore the `isolatedModules` flag (which was removed
as redunant).

See:

* https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#verbatimmodulesyntax
* microsoft/TypeScript#52203
* tommy351/tsc-multi#19
trevor-scheer added a commit to apollographql/apollo-server that referenced this pull request Mar 29, 2023
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>
milesrichardson added a commit to splitgraph/madatdata that referenced this pull request Apr 10, 2023
…roken

Need to make some decisions about whether/how to emit CJS modules,
because as long as we have verbatimModuleSyntax enabled, we cannot
use the typical `export const = "x"` format, and tsc-multi rewriting
is also now breaking for this on v5, and patching it to do the rewrites
may be non trivial since it can involve shifting large blocks of code.

Alternatively we could just not opt into the flag, in which case
we should also restore the `isolatedModules` flag (which was removed
as redunant).

See:

* https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#verbatimmodulesyntax
* microsoft/TypeScript#52203
* tommy351/tsc-multi#19
linhe0x0 added a commit to linhe0x0/tsconfig that referenced this pull request Apr 17, 2023
becaure --importsNotUsedAsValues are being deprecated, more details can
be found at microsoft/TypeScript#52203
trevor-scheer added a commit to apollographql/apollo-server that referenced this pull request Apr 17, 2023
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 added a commit to apollographql/apollo-server that referenced this pull request Apr 17, 2023
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>
cloudcome added a commit to FrontEndDev-org/openapi-axios that referenced this pull request Apr 24, 2023
mnapthine added a commit to action-is-hope/shelley that referenced this pull request May 19, 2023
ras0q added a commit to traPtitech/traQ_S-UI that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: deprecate importsNotUsedAsValues and preserveValueImports in favor of single flag
7 participants