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: Encode and read emit-affecting compiler options in declaration files #54212

Open
andrewbranch opened this issue May 10, 2023 · 1 comment
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@andrewbranch
Copy link
Member

andrewbranch commented May 10, 2023

TL;DR

Put emit-affecting options at the top of .d.ts files like

// @esModuleInterop: true
// @module: commonjs
// @preserveConstEnums: true
// @useDefineForClassFields: true

so we can know what’s going on in JS files when we read in .d.ts files from external dependencies.

Super basic background

Type checking is designed to reason about what your JS code will do at runtime. When you change a compiler option that affects what JS code gets emitted, there is often a corresponding change to type checking behavior. For example, when emitting CommonJS modules, the esModuleInterop changes the JS emit for default imports so they can be used in more situations. Because of this emit change, a compiler error can go away:

import express from "express";
//     ^^^^^^^
// This module is declared with 'export =', and can only be used
// with a default import when using the 'esModuleInterop' flag.

Here, the compiler knows that the JS generated for the default import will not work with the "express" module. But if we enable esModuleInterop, the generated JS changes in a way that makes it compatible, and consequently the type checking error goes away. The presence or absence of the error is driven by accurate knowledge of the runtime JS.

The problem

When a project consumes a type declaration file, say from an external dependency, the declaration file is supposed to contain everything the compiler needs to know about the JS file it represents—no more and no less. But declaration files typically don’t change in any way as a result of modifying emit-affecting settings. The compiler makes assumptions about the contents of the corresponding JS files based on the compiler options of whatever project consumes the declaration files. Since type checking behavior is driven by knowledge about the runtime behavior of JS files, type checking can be incorrect when consuming libraries compiled with emit-affecting options different from the consuming project’s options.

Returning to the esModuleInterop example, this issue has led to conventional wisdom like “libraries should not use esModuleInterop.” The idea is that if a library uses esModuleInterop to silence an error on a default import like the one shown above, then publishes type declaration files containing that default import, then every user who consumes those declarations must also enable esModuleInterop. This conventional wisdom is probably the best we can do currently, but it’s also deeply flawed. Compiling without esModuleInterop also allows libraries to write (categorically incorrect!) code that produces errors when compiled with esModuleInterop:

import * as express from "express";
export type ExpressArgs = Parameters<typeof express>;
//                                   ^^^^^^^^^^^^^^
// Type 'typeof e' does not satisfy the constraint '(...args: any) => any'.
//  Type 'typeof e' provides no match for the signature '(...args: any): any'.(2344)
//
// index.d.ts(1, 1): Type originates at this import. A namespace-style import cannot be called
// or constructed, and will cause a failure at runtime. Consider using a default import or
// import require here instead.

The issue is bidirectional; neither value of esModuleInterop guarantees that consumers will not have to adopt the setting used by the library.

The problem is not unique to esModuleInterop; it applies to nearly all emit-affecting compiler options that result in a corresponding checking change. Other particularly relevant ones that come to mind are:

  • useDefineForClassFields
  • preserveConstEnums
  • module

Other emit-affecting options are either not semantically meaningful (removeComments) or tend to be relevant only in implementation files (downlevelIteration, jsxImportSource).

Proposal

I suggest encoding these relevant options into declaration files when emitting them, and reading and respecting them during checking. I’m not particular about syntax, but it needs to be human-writable so it can be used on DefinitelyTyped. Both the parsing and likely much of the checking infrastructure could be reused from #49886, so I would advocate for reusing that syntax:

// @esModuleInterop: true

These directives would be unconditionally added to the top of all emitted declaration files. Any directive-controllable options that are not set with a directive should fall back to today’s behavior (use the consuming project’s options). Note that this means we cannot assume a default value for directives and elide ones that would be redundant with that default, since no directive means “use the user’s config,” which could have a value that disagrees with whatever default we set. So all options we think are relevant should always be encoded in declaration file emit.

What about project references?

Project references on the command line consume declaration files, while in TS Server, they use the referenced project’s source .ts files. The checker may need some changes to ensure that the value of directive-controllable options are read from the referenced project’s tsconfig.

People hand-writing definitions will mess it up!

The potential for messing up hand-written declarations is already so high that adding one more degree of freedom doesn’t move the needle at all. Careful checks will need to be implemented on DefinitelyTyped. Everyone else is on their own. I believe this will solve more problems than it creates.

Does this conflict with #49886?

I don’t think so. I suggest making such directives errors in implementation files, which may help preserve that space for #49886 if we find a way to make it viable performance-wise. Currently, none of the options I identified as interesting are targeted in #49886. If one of them was able to be overridden in an implementation file in the future, the directive would just have to be emitted as-is to the declaration file.

Will this affect performance?

I hope that the fact that this targets much fewer options than #49886, and ones that are checked somewhat less often than strictness flags, means the performance penalty will be negligible. But experimentation will be needed.

Alternatives

You can imagine a lot of ways to put something in a declaration file to encode extra information about imports, class fields, const enums, and others. I’m open to discussing very different syntax and potentially tackling a narrower scope than what I’ve discussed (I’m mostly invested in fixing esModuleInterop at the moment), but I can’t think of any way to do it without changing declaration file emit in some way. I think a file-level directive has merit because I think it can leverage existing work from #49886, and the settings cannot be changed more granularly than file-level anyway.


Related: #52779, some const enum ones, surely others

@andrewbranch andrewbranch added Suggestion An idea for TypeScript In Discussion Not yet reached consensus Domain: Declaration Emit The issue relates to the emission of d.ts files labels May 10, 2023
@weswigham
Copy link
Member

Just to reiterate what I've said that'll end up in the design meeting notes, I don't really like the idea of encoding this in comments just because we fear a break. This is definitely semantic information, and should be treated appropriately. Moreover, any "syntax" (comment or otherwise) people are going to try and use/abuse in non-declaration files. It would be helpful if we could actually error on that case, which would be troublesome for a comment (erroring on a comment never feels right - it's a decent indicator it shouldn't have been a comment if it could be avoided).

IMO, it would also be useful to be able to say "resolve this import as though it had an interop helper" for any type-only import, not just declaration file ones. Like, if you write
import type mod from "x" in a non-esModuleInterop project, but you want the interop import behavior (because, eg, that's what "x"'s docs say you should do), it's pretty reasonable to say that you want the helper behavior applied to the import. In real JS, you could always explicitly tslib.__importDefault(mod) (or, more likely, hardcode mod = mod.default) as a one-off, but there's no type equivalent (mostly because type constructors can't transform namespaces easily).

I'd be much more comfortable with an explicit syntax on the import (eg, a with attribute) that we can allow on all non-implementation imports. We already have tools like downlevel-dts and typesVersions to make changes like this more palatable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

2 participants