Closed andrewbranch closed 1 year ago
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"
?
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.
We agreed to move forward with this.
verbatimModuleSyntax
, just because it’s more understandable than isolatedModules
. It will imply/supersede isolatesModules
.--module commonjs
(I think @jakebailey has investigated this a bit, but I don’t recall the conclusion)import type
but are not (the functionality we lose from --importsNotUsedAsValues error
)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.
Yeah, I think that’s reasonable.
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
.
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 🙃
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 😂
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.
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.
import fs = require("fs");
Then why not this?
const fs = require("fs");
@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.
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. (https://github.com/typescript-eslint/typescript-eslint/issues/6379)
Is there any specific reason that it was decided that TS wouldn't "roll-up" the elison?
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.
@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.
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.
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.
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.
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?
Discussing the best course of action here: https://github.com/import-js/eslint-plugin-import/issues/2676#issuecomment-1407107260
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.
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.)
@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.
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"
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".
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
.
@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?)
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.
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.
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 theirimport
andexport
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")
andexport = {}
). 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.
I found it! https://github.com/microsoft/TypeScript/issues/53683
Working as expected 😄 And the moduleDetection
flag could be set to "force" to treat files as modules by default.
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.
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!
Background:
importsNotUsedAsValues
importsNotUsedAsValues
was introduced alongside type-only imports in https://github.com/microsoft/TypeScript/pull/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: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 topreserve
, 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 https://github.com/microsoft/TypeScript/pull/44619 as a way to control elision of individual imported names so that symbols can be referenced from places TypeScript cannot analyze, likeeval
statements or Vue templates:Under default compiler options, the entire import statement is removed, so the eval’d code fails. Under
--importsNotUsedAsValues preserve
, the import declaration is preserved asimport "./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
andpreserveValueImports
with a single flag that isOn the schedule of https://github.com/microsoft/TypeScript/issues/51000, I propose deprecating
importsNotUsedAsValues
andpreserveValueImports
, and replacing them with a single flag called (bikesheddable)verbatimModuleSyntax
. The effect ofverbatimModuleSyntax
can be described very simply:No elision without
type
This is a stricter setting than either
importsNotUsedAsValues
orpreserveValueImports
(though it’s approximately what you get by combining both withisolatedModules
), because it requires that all types be marked as type-only. For example:would be an error in
--verbatimModuleSyntax
becauseWriteFileOptions
is only a type, so would be a runtime error if emitted to JS. This import would have to be writtenNo 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:This import is legal under
--module esnext
, but an error in--module commonjs
. (Innode16
andnodenext
, 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 asinstead. 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 outrequire
helps you understand when and why you can’trequire
an ES module—it’s easier to lose track of this when yourrequire
is disguised as an ESMimport
in the source file.