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

Declaration emit should not inline type definitions #37151

Open
mheiber opened this issue Mar 2, 2020 · 20 comments
Open

Declaration emit should not inline type definitions #37151

mheiber opened this issue Mar 2, 2020 · 20 comments
Assignees
Labels
Needs Investigation This issue needs a team member to investigate its status.

Comments

@mheiber
Copy link
Contributor

mheiber commented Mar 2, 2020

TypeScript Version: 3.8.3, 3.8.1, probably others

Search Terms:

declaration inlining, dts inlining, declaration inline, inline literal, declaration literal

Code

// parent.ts
import { num, obj } from "./child"
export const reExportNum = num;
export const reExportObj = obj;
// child.d.ts
export declare const num: number;
export declare const obj: { a: 1 };

tsc index.ts --declaration

Expected behavior:

Declaration emit for parent.ts should not inline types.

// parent.d.ts
import { num, obj } from "./child"
export declare const reExportNum: typeof num;
export declare const reExportObj: typeof obj;

Actual behavior:

Today, declaration emit for parent.ts inlines the types and eliminates the import of the child.d.ts type definition.

// parent.d.ts
export declare const reExportNum: number;
export declare const reExportObj: {
    a: 1;
};

This is a correctness issue, because consumers of parent.d.ts will not get the correct types if the types in child.d.ts change.

In practice, this is most likely to happen when parent and child are in separate packages, because they are published independently, i.e. an application uses parent-package which uses types from child-package. This is exacerbated by the current practice on npm of parent-package depending on an unpinned version, using package.json dependency syntax "child-package": "*".

This issue was co-authored with @robpalme

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Mar 3, 2020

What would you have TypeScript emit in the case of the following?

export const exportNumNum = num + num;

export const exportExtendedObj = {
  ...obj,
  someOtherProp: 200,
};

@mheiber
Copy link
Contributor Author

mheiber commented Mar 4, 2020

Thanks for looking into this, @DanielRosenwasser . As your examples illustrate, there is a long tail of correctness issues in this area.

If examples like the one in the issue description are the common case, maybe it would be possible to solve them independently of the uncommon cases.

We have some ideas for how to deal with the less common cases and can open a separate issue for them if you think it's a good idea. Perhaps related: #29043.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Mar 4, 2020
@RyanCavanaugh
Copy link
Member

There's a fairly fundamental tension here about what declaration emit means: Should declaration files represent the types as they existed when you compiled your program, or should they represent the types that a consuming library would have seen had your original program been compiled "in the context of" the consumer's setup?

This gets really mind-bending if you think about conditional types or overloads, e.g.

declare var x: SomeType;
export const c = func(x);

Is the intent here that c should have the type that you saw when you invoked func? Or if a consuming library augments SomeType in a way that changes the resulting type of func(x), should c have some other new type? What if that causes some other use of c to break a contract?

I think one of these behaviors is much easier to reason about than the other, as you can probably tell from my descriptions of them.

Anyway the example in the OP is also in tension with people who want their end-result .d.ts file to be a single-ish artifact that doesn't expose their entire program's internal structure. This is a good goal anyway for performance - it'd be much better if we load 1 file per library instead of 12, and better if we handle some inline anonymous types instead of resolving typeof queries everywhere.

@mheiber
Copy link
Contributor Author

mheiber commented Mar 5, 2020

reply to #37151 (comment)

@RyanCavanaugh thanks for your response, I find your description of the two mental models really helpful: the "types are conceptually inlined" model vs. the "types always flow through" model.

I'm not sure the inlining is easier to reason about, given that "types flow through" is how .ts files work and is also similar to how the runtime works:

export { foo } from "./foo"; // re-exposes whatever is in ./foo

It also seems like the inlining model doesn't work for nominal types: it would lead to spurious type incompatibility errors. That's probably why TS currently does not inline nominal types.

So if the choices are:

  • two mental models, one of which leads to runtime errors
  • a single mental model in which types are correct

I'd choose the latter. Please let me know if this is a misleading way of stating the choices!

@mheiber
Copy link
Contributor Author

mheiber commented Mar 16, 2020

I'm curious about this issue: is there a way to help progress this?

I could open a separate issue for @RyanCavanaugh 's more fundamental topic re the point of a declaration file.

@mheiber
Copy link
Contributor Author

mheiber commented Mar 31, 2020

Following up on this issue: it's causing us some pain, so it would be good to have some guidance.

@RyanCavanaugh
Copy link
Member

@mheiber have you tried with @next lately? We recently had some PRs go through that might improve the emit to more closely match your expecations.

@weswigham
Copy link
Member

If you're thinking of #37444, that's still out for review.

@mheiber
Copy link
Contributor Author

mheiber commented Mar 31, 2020

Thanks Ryan and Wes! I'll check out Wes' PR.

@mheiber
Copy link
Contributor Author

mheiber commented Mar 31, 2020

Thanks for the recommendation. I tried #37444 and am not seeing a difference in the output for the example above.

@weswigham
Copy link
Member

Yeah, didn't think it would - you're looking for everything to be represented with typeof queries and indexed accesses where possible, essentially. Which is just plain not something we even track the information to do right now.

Naturally, you can always just write the type annotations yourself if preserving that last bit of origin information is important (eg, because you expect augmentations somehow).

@mheiber
Copy link
Contributor Author

mheiber commented Apr 1, 2020

@weswigham thanks for explaining. After looking at your related PR, I think I can see how the information is not currently tracked.

Regarding design (rather than implementation), the fundamental issue seems to me to be that declaration files are neither:

  • fully dynamic: types always "flow through" from transitive dependencies, just like values flow through at runtime
  • fully static: types are always fully inlined

I gave some reasons above why I see advantages to the dynamic model (#37151 (comment)). One of the most compelling reasons, in my opinion, is that it's the only way I can see that will work well with nominal types.

Am I understanding the design issue correctly?

@mheiber
Copy link
Contributor Author

mheiber commented Jun 24, 2020

@DanielRosenwasser re:

What would you have TypeScript emit in the case of the following?

export const exportNumNum = num + num;

export const exportExtendedObj = {
  ...obj,
  someOtherProp: 200,
};

A superpowered typeof that works for arbitrary expressions would solve this problem:

export declare const exportNumNum: typeof num + num;

Would also address:

Would typeof with arbitrary expressions be worth considering in a separate issue? @dragomirtitian experimented with implementing this before.

@cyberixae
Copy link

I'm here because inlining everything caused my .d.ts file to be over 6.5MB long. The type checks started failing and I am wondering if the compiler simply ignores the end of the file because of some limitation.

@robpalme
Copy link

@cyberixae One mitigation to reduce inlining is to use interface rather than type when defining object shapes. This causes tsc to reference the original type by name, which may further cause it to generate type-only import() expressions.

@aleksey-ilin
Copy link

@mheiber Have you find a solve for this problem?

@mheiber
Copy link
Contributor Author

mheiber commented Dec 19, 2020

@aleksey-ilin I'm not writing TS full-time anymore, @robpalme is more up to date. But my understanding is that this is a fundamental issue with TS not picking a consistent model for type inlining.

@robpalme
Copy link

robpalme commented Jan 7, 2021

@aleksey-ilin the main solution I have found to solve huge declarations is to identify the root type that is inlined and then, assuming it is a statically known object type, create an interface from it and then refer to that interface at all usage sites.

export interface WrappedProblemType extends ProblemType {}

I have been experimenting with changing declaration emit so that shenanigans like this are not necessary. It kinda works and I'll share that soon.


Separately, union and intersection types also get inlined. interface will not save you in this case - there is no reliable userland workaround for these. Thankfully there is work in progress to reduce the inlining of these specific types in #42149

@electrovir
Copy link

electrovir commented Sep 15, 2024

The fact that the TypeScript compiler does this has caused many issues for me, including:

  1. JSDoc comments on the original code are not preserved in the inlined types. (example, every method is missing its JSDoc comment)
  2. Large union type aliases that get inlined (multiple times throughout the project) explode the declaration file size and make editor tooltips impossible to read. (example, scroll to the right)
  3. Sometimes type imports from dependencies are wiped out entirely, breaking everything. (example, fixed by writing my own declaration file here)

@robpalme
Copy link

Thank you for the examples.

There's more work on the way related to Isolated Declarations that may mitigate 1 and 2.

3 just sounds like a bug. If you have a small repro, please file a standalone issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

No branches or pull requests

8 participants