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

Proposal: deprecate importsNotUsedAsValues and preserveValueImports in favor of single flag #51479

Closed
andrewbranch opened this issue Nov 10, 2022 · 31 comments �� Fixed by #52203
Closed
Assignees
Labels
Committed The team has roadmapped this issue Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript

Comments

@andrewbranch
Copy link
Member

Background: importsNotUsedAsValues

importsNotUsedAsValues was introduced alongside type-only imports in #35200 as a way to control import elision. In particular, Angular users often experienced runtime errors due to the unintended import elision in files like:

import { MyService } from './MyService';

class MyComponent {
  constructor(private myService: MyService) { }
}

It appears to TypeScript as if the import declaration can be elided from the JS emit, but the ./MyService module contained order-sensitive side effects. By setting the new --importsNotUsedAsValues flag to preserve, import declarations would not be elided, and the module loading order and side effects could be preserved. Type-only imports could then be used to elide specific import declarations.

Background: preserveValueImports

preserveValueImports was added in #44619 as a way to control elision of individual imported names so that symbols can be referenced from places TypeScript cannot analyze, like eval statements or Vue templates:

import { doSomething } from "./module";

eval("doSomething()");

Under default compiler options, the entire import statement is removed, so the eval’d code fails. Under --importsNotUsedAsValues preserve, the import declaration is preserved as import "./module" since the flag is only concerned with module loading order and potential side effects that may be contained in "./module". Under the new --preserveValueImports option, doSomething would be preserved even though the compiler thinks it is unused.

In the same release, the ability to mark individual import specifiers as type-only was added as a complement to --preserveValueImports.

User feedback

These two flags, along with type-only import syntax, were designed to solve fairly niche problems. Early on, I encouraged users not to use type-only imports unless they were facing one of those problems. But as soon as they were available, and consistently since then, we have seen enthusiasm for adopting type-only imports everywhere possible as an explicit marker of what imports will survive compilation to JS. But since the flags were not designed to support that kind of usage of type-only imports, the enthusiasm has been accompanied by confusion around the configuration space and frustration that auto-imports, error checking, and emit don’t align with users’ mental model of type-only imports.

Further, because the two flags were designed at different times to address different issues, they interact with each other (and with isolatedModules) in ways that are difficult to explain without diving into the background of each flag and the narrow problems they were intended to solve. And the flag names do nothing to clear up this confusion.

Proposal

We can solve the problems addressed by importsNotUsedAsValues and preserveValueImports with a single flag that is

  • easier to explain
  • less complex to implement
  • well-suited to users who want to use type-only imports for stylistic reasons

On the schedule of #51000, I propose deprecating importsNotUsedAsValues and preserveValueImports, and replacing them with a single flag called (bikesheddable) verbatimModuleSyntax. The effect of verbatimModuleSyntax can be described very simply:

verbatimModuleSyntax: Emits imports and exports to JS outputs exactly as written in input files, minus anything marked as type-only. Includes checks to ensure the resulting output will be valid.

No elision without type

This is a stricter setting than either importsNotUsedAsValues or preserveValueImports (though it’s approximately what you get by combining both with isolatedModules), because it requires that all types be marked as type-only. For example:

import { writeFile, WriteFileOptions } from "fs";

would be an error in --verbatimModuleSyntax because WriteFileOptions is only a type, so would be a runtime error if emitted to JS. This import would have to be written

import { writeFile, type WriteFileOptions } from "fs";

No transformations between module systems

True to its name, verbatimModuleSyntax has another consequence: ESM syntax cannot be used in files that will emit CommonJS syntax. For example:

import { writeFile } from "fs";

This import is legal under --module esnext, but an error in --module commonjs. (In node16 and nodenext, it depends on the file extension and/or the package.json "type" field.) If the file is determined to be a CommonJS module at emit by any of these settings, it must be written as

import fs = require("fs");

instead. Many users have the impression that this syntax is legacy or deprecated, but that’s not the case. It accurately reflects that the output will use a require statement, instead of obscuring the output behind layers of transformations and interop helpers. I think using this syntax is particularly valuable in .cts files under --module nodenext, because in Node’s module system, imports and requires have markedly different semantics, and actually writing out require helps you understand when and why you can’t require an ES module—it’s easier to lose track of this when your require is disguised as an ESM import in the source file.

@andrewbranch andrewbranch added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Nov 10, 2022
@robpalme
Copy link

This is great. Simplifying the configuration to achieve the more explicit/understandable mental model will really help adoption.

As a thought exercise, I wonder if we can constrain the configuration even more to reduce combinatorial tsconfig complexity. Does this new flag need to be independent of isolatedModules? Would it ever make sense to use the option without isolatedModules?

Concretely, could we express the new option as "isolatedModules": "strict"?

@andrewbranch
Copy link
Member Author

Yeah, I think the flag as I’ve described it is a superset of isolatedModules, with the lone exception of a rule against accessing ambient const enums in isolatedModules. The const enum interaction with other flags has gotten incredibly messy and it’s not clear to me what’s needed there. But making this behavior a variant of isolatedModules seems doable. My only regret is that isolatedModules is itself not a very descriptive name, but I can live with that.

@andrewbranch andrewbranch added Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Dec 2, 2022
@andrewbranch andrewbranch self-assigned this Dec 2, 2022
@andrewbranch andrewbranch added this to the TypeScript 5.0.0 milestone Dec 2, 2022
@andrewbranch
Copy link
Member Author

We agreed to move forward with this.

  • The flag will be called verbatimModuleSyntax, just because it’s more understandable than isolatedModules. It will imply/supersede isolatesModules.
  • We would like to adopt the flag in our own codebase, but I think that may not be practical until/unless we can move off --module commonjs (I think @jakebailey has investigated this a bit, but I don’t recall the conclusion)
  • We should make sure typescript-eslint can identify imports that can be import type but are not (the functionality we lose from --importsNotUsedAsValues error)
  • The codefix/organize imports/auto imports experience needs to be really good.
@robpalme
Copy link

robpalme commented Dec 3, 2022

I'm very happy this is going ahead.

Given the new flag now obsoletes three existing flags, could we reduce config complexity by erroring if any of the existing flags are used in conjunction with the new flag regardless of their value? Meaning this would force users to clean up and remove those old entries in tsconfig and would shortcut any discussion of how they interact.

@andrewbranch
Copy link
Member Author

Yeah, I think that’s reasonable.

@jakebailey
Copy link
Member

We would like to adopt the flag in our own codebase, but I think that may not be practical until/unless we can move off --module commonjs (I think @jakebailey has investigated this a bit, but I don’t recall the conclusion)

My memory is bad, but given I was able to convert the entire codebase to pure ESM with esnext (as part of early module conversion perf testing), I think we could change this, yes, if we continue to use a bundler. Once there's a PR it should be straightforward to test, I think. But, I don't quite know what will happen to our non-bundler case, because that definitely needs --module commonjs.

@andrewbranch
Copy link
Member Author

Well, if we have multiple configs for multiple compilations, the one that has --module commonjs can simply not enable verbatimModuleSyntax. But ironically, the purported value of the flag is emit transparency, which is exactly what we don’t have by using a bundler, and what we might think valuable in our Node build 🙃

@Jack-Works
Copy link
Contributor

The flag will be called verbatimModuleSyntax, just because it’s more understandable than isolatedModules.

as a non-native, this is the first time I see the word "verbatim" so it's not so understandable to me 😂

@bradzacher
Copy link
Contributor

We should make sure typescript-eslint can identify imports that can be import type but are not

consistent-type-imports does what you want for 99.9% of the cases.

There are some difficulties around decorators + emitDecoratorMetadata because TS has changed over time and does have some magic where cross-module types can impact the compiler output, which we cannot replicate as the rule is not type-aware.

Worth noting that we also have consistent-type-exports which enforces that types are exported with a specifier. It is type-aware so it can handle the export-with-source and import-then-export cases.

Together these rules should be enough to enforce that a module can be safely transpiled in isolation.

@bradzacher
Copy link
Contributor

Is it possible to also get this added to the web playground?
I wanted to play around with it - but it's not listed as an option in the UI.
I could probably hack around to enable it - but it'd be good to see it added first-class to repro things easily.

Note that importsNotUsedAsValues was also not added to the playground web playground, but preserveValueImports was.

@Jack-Works
Copy link
Contributor

import fs = require("fs");

Then why not this?

const fs = require("fs");

@andrewbranch
Copy link
Member Author

@bradzacher You can control any option in the playground with a magic comment: // @verbatimModuleSyntax: true (note that it’s “sticky” and stays true even if deleted; you have to switch it to false to revert).

@Jack-Works a const declaration only declares a value, whereas an import alias can declare a value, type, namespace, or some combination of them. So without the import keyword, it would be impossible to, say, use the type meaning of a class:

const User = require("./models/user");
const u: User = new User();
//       ^^^^ `User` is only a value

We could say that you can use const when you only need a value and import when you need types, but it would be a massive breaking change to start resolving const/require in *.ts files. Lots of people use const/require as an escape hatch when they need to get a dependency whose typings are totally busted.

I do wish TypeScript had come up with a way to use totally standard CJS syntax and still be able to pass types around from the beginning, but (1) I have thought about this a lot and have never come up with a great alternative, and (2) I think the ship sailed too long ago to be worth trying to revise it now.

@bradzacher
Copy link
Contributor

Whilst I love the idea behind this, the one part of it I simply cannot get behind is this behaviour:

import {type T} from 'mod';

// becomes

import 'mod';

I get that the idea is that TS will only operate on the specifier, but it just seems so wrong to me that an import that only imports types with inline specifiers would be left behind to create a runtime side-effect.
It feels wrong because that same import with the keyword shifted one token to the left would be entirely elided.

This seems like it's going to just exist as a foot-gun for users that will need to be solved via linting.
There are already requests to change/add lint rules to catch this case because some users can see that it's not the output they would want. (typescript-eslint/typescript-eslint#6379)

Is there any specific reason that it was decided that TS wouldn't "roll-up" the elison?

@Jack-Works
Copy link
Contributor

You can think

import {type T} from 'mod';

// becomes

import {} from 'mod';

This is the same as import 'mod'. TS current behavior is correct IMO.

@bradzacher
Copy link
Contributor

@Jack-Works to clarify, i fully understand the steps to get to a side-effect import. The crux of it is that TS elided inline specifiers without touching the declaration.

The question I'm asking is why it was chosen that TS wouldn't roll up the elison to the declaration like all the prior art does eg babel (which is the same result for TS and flow), TS's default behaviour, swc.

@andrewbranch
Copy link
Member Author

Some users need control over whether the declaration is preserved for module-loading side-effects. That’s why importsNotUsedAsValues was made, so its successor needs to have feature parity. Also, the point of the flag is to be obvious and explainable. We only mess with the parts that have type on them. No more magic.

When designing this feature, I fully expected linters to pick up the job of making sure import declarations are marked as type-only when possible for users who want to ensure that no side-effect imports remain unnecessarily in their emit, as I said in the proposal:

We should make sure typescript-eslint can identify imports that can be import type but are not

and I was pleasantly surprised when you commented earlier that consistent-type-imports would be sufficient here.

@andrewbranch
Copy link
Member Author

Also, it doesn’t really matter for what people care about here, but the blog is currently incorrect. import {type T} from 'mod' becomes import {} from 'mod', not import 'mod'. I clarify this just to reinforce the point that the syntactic transformation is dead-simple, and does what it looks like it would do, in my opinion.

@bradzacher
Copy link
Contributor

bradzacher commented Jan 27, 2023

I was pleasantly surprised when you commented earlier that consistent-type-imports would be sufficient here

Reading the OP and the thread I didn't see any mention of this behaviour and given that the base behaviour of TS is to not do this, I didn't know that importsNotUsedAsValues worked this way (I've never used the flag, personally). Which is why I said the lint rules would cover things.

consistent-type-imports will enforce that any import specifier used only in a type location must be marked with a type specifier. That is all that it does. It does not encode logic to ensure that an import that has no value specifiers should use a top-level qualifier because as far as we knew this was a purely stylistic choice.
From talking to the community it seems most people still believe it is (which is technically correct... In some cases).

There are other lint rules that can enforce whether you use top-level or inline specifiers, but none so far that enforce that allow inline specifiers, but enforce that a top-level qualifier be used if there are no value specifiers.

@andrewbranch
Copy link
Member Author

but none so far that enforce that allow inline specifiers, but enforce that a top-level qualifier be used if there are no value specifiers

Would you be open to including such a rule in typescript-eslint?

@bradzacher
Copy link
Contributor

Discussing the best course of action here:
import-js/eslint-plugin-import#2676 (comment)

@ljharb
Copy link
Contributor

ljharb commented Jan 27, 2023

Side-effecting imports, by almost universal convention, do not export anything - as such, it's advisable to assume that there are only side effects if the file is imported without bindings, including types. An import statement that only brings in types should mean that the entire import statement is removed, so that the file isn't included at all.

@jakebailey
Copy link
Member

jakebailey commented Jan 27, 2023

Just to look at this at another point of view; if we use the same sort of type-erasure scheme as the "types in JS" ECMAScript proposal uses, this code:

import { type Foo } from "bar";

Would be read by the runtime as:

import {} from "bar";

Because only the types get erased. If I were to instead write import type { Foo } from "bar", the entire import is a type import, so fully gets erased.

That means the question "should this module be loaded?" becomes "does the import declaration have a type keyword?". To me this is a simple rule, compared to "and also, if all specifiers have type, then also do nothing", and less likely to break things (as mentioned in the original PR description).

(But, this is of course my own mental model of things.)

@ljharb
Copy link
Contributor

ljharb commented Jan 27, 2023

@jakebailey i agree the most conservative approach would do exactly this - but I don't think it's actually beneficial for the ecosystem to tacitly endorse modules with exports also having side effects, by being concerned with breaking those use cases.

@robpalme
Copy link

Just exploring the auto-removal-if-no-value-bindings suggestion, what runtime behavior would we expect from these files? Should they match?

// index.js
import {} from "path" 
// index.ts
import {} from "path" 
@ljharb
Copy link
Contributor

ljharb commented Jan 27, 2023

Those specific two, sure. But import { type x } from 'path' is NOT simply import {} from 'path' with a type import added on, it's "i like that style better and i only added this import statement to get the type".

@andrewbranch
Copy link
Member Author

For a bit of historical context, the pattern that motivated importsNotUsedAsValues was from an older version of Angular, where it was indeed very common to export things and have side effects in the way of DI registration. TypeScript has always preserved side-effect-only imports (ones with no import clause whatsoever); the complaint was that code like this was needed in order to ensure SomeService got registered in the right order or something:

import { SomeService } from "./service"; // gets removed completely
import "./service";                      // always stays

export class SomeComponent {
  constructor(service: SomeService) {}
}

it's "i like that style better and i only added this import statement to get the type".

Completely disagree. Different syntax was added for different use cases and has different behavior.

Also, just to make sure everyone realizes, none of this is new with verbatimModuleSyntax. Elision has always worked this way with importsNotUsedAsValues and preserveValueImports.

@ljharb
Copy link
Contributor

ljharb commented Jan 27, 2023

@andrewbranch why the syntax was added is never guaranteed to be the same reason someone chooses to use it.

I would argue that the two-line approach is hugely preferable, and even better would be avoiding that hazardous design pattern in angular itself (which "older" implies, they did move to avoid?)

@andrewbranch
Copy link
Member Author

andrewbranch commented Jan 28, 2023

I used to work on a big web app with an absolute spaghetti of cyclic dependencies where everything constructed singleton classes at load time that all depend on each other. Having been scarred for life, I don’t need convincing that the pattern is bad 😄. But Angular version whatever-point-oh was pretty popular, and it was one of our most upvoted issues.

But I don’t think we have to justify past motivations to make sense of this. verbatimModuleSyntax is an opt-in flag that makes imports and exports emit as close to what you wrote as possible. If you want imports to be elided wherever possible, that’s literally the default. If you want that, but also want to be transpiler-friendly, that is and always has been isolatedModules. Neither of these modes is going away. 99% of people who were using importsNotUsedAsValues were using it for linty reasons. People who want that can use a linter.

My top priority is to keep verbatimModuleSyntax simple and explainable, which means not changing the emit back to do magic elision. What I’m potentially more willing to budge on is whether there should be a way of keeping the error that importsNotUsedAsValues had that makes sure you write import type (fully elided import declarations) whenever possible. I thought that concern was kind of linty, and if we issued an error for it predicated on verbatimModuleSyntax, we would be un-fixing the old Angular-inspired issue we fixed. Those users would have to go back to using double imports, or // @ts-ignore the error. We could error under a separate flag, but I really hoped to reduce the complexity matrix here. While I haven’t really heard any compelling reasons why this isn’t a lint issue, I’m open to feedback on that.

@zanminkian
Copy link

import= and export= syntax are unusable for development experience reason.That means, developers cannot enable verbatimModuleSyntax if they are in commonjs, because no one use import= and export= syntax even they compile the ts to commonjs. Developers who are in esm should enable this flag.

This flag is really unfriendly to commonjs project.

@andrewbranch
Copy link
Member Author

Yep that’s what the PR says. It’s also what the new in-progress module docs say.

In TypeScript 5.0, a new compiler option called verbatimModuleSyntax was introduced to help TypeScript authors know exactly how their import and export statements will be emitted. When enabled, the flag requires imports and exports in input files to be written in the form that will undergo the least amount of transformation before emit. So if a file will be emitted as ESM, imports and exports must be written in ESM syntax; if a file will be emitted as CJS, it must be written in the CommonJS-inspired TypeScript syntax (import fs = require("fs") and export = {}). This setting is particularly recommended for Node projects that use mostly ESM, but have a select few CJS files. It is not recommended for projects that currently target CJS, but may want to target ESM in the future.

@icyJoseph
Copy link

icyJoseph commented Jun 2, 2023

Edit

I found it! #53683

Working as expected 😄 And the moduleDetection flag could be set to "force" to treat files as modules by default.

OP

I am really not sure if this is the forum/thread for this, but it is what I was able to trace using --isolatedModules in the switch from 4.9.5 to 5.0.x.

Given this file:

// I am aware that either is enough to raise an error up to 4.9.5
interface Foo {
  bar(): void;
}

function foo() {}

When using 4.9.5 it causes this, expected, error:

foo.ts:1:1 - error TS1208: 'foo.ts' cannot be compiled under '--isolatedModules' because it is considered a global script file. Add an import, export, or an empty 'export {}' statement to make it a module.

1 interface Foo {
  ~~~~~~~~~


Found 1 error in foo.ts:1

So far so good. Now with the same tsconfig.json, upgrading to anything on the 5.x.x range, has stopped raising the error, and the code compiles normally.

tsconfig

The tsconfig.json file used is the tsc --init default with this modification:

+ "isolatedModules": true,

Perhaps any of you can recognise this change in behavior? Perhaps it is expected to begin with? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
9 participants