-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Comments
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 Concretely, could we express the new option as |
Yeah, I think the flag as I’ve described it is a superset of |
We agreed to move forward with this.
|
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 |
Yeah, I think that’s reasonable. |
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 |
Well, if we have multiple configs for multiple compilations, the one that has |
as a non-native, this is the first time I see the word "verbatim" so it's not so understandable to me 😂 |
There are some difficulties around decorators + Worth noting that we also have 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? Note that |
Then why not this?
|
@bradzacher You can control any option in the playground with a magic comment: @Jack-Works a const User = require("./models/user");
const u: User = new User();
// ^^^^ `User` is only a value We could say that you can use 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. This seems like it's going to just exist as a foot-gun for users that will need to be solved via linting. 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 |
@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 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:
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. |
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
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. |
Would you be open to including such a rule in typescript-eslint? |
Discussing the best course of action here: |
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 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 (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 |
For a bit of historical context, the pattern that motivated import { SomeService } from "./service"; // gets removed completely
import "./service"; // always stays
export class SomeComponent {
constructor(service: SomeService) {}
}
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 |
@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. My top priority is to keep |
This flag is really unfriendly to |
Yep that’s what the PR says. It’s also what the new in-progress module docs say.
|
EditI found it! #53683 Working as expected 😄 And the OPI am really not sure if this is the forum/thread for this, but it is what I was able to trace using 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 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 tsconfigThe + "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 #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 #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 #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.The text was updated successfully, but these errors were encountered: