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

[transpileDeclaration API][5.5] More missing type-only imports #58511

Closed
MichaelMitchell-at opened this issue May 13, 2024 · 4 comments Β· Fixed by #58539
Closed

[transpileDeclaration API][5.5] More missing type-only imports #58511

MichaelMitchell-at opened this issue May 13, 2024 · 4 comments Β· Fixed by #58539
Labels
Domain: ts.transpileDeclaration Issues regarding the transpileDeclaration API, which do not reproduce without it

Comments

@MichaelMitchell-at
Copy link

πŸ”Ž Search Terms

transpiledeclaration api emit missing import

πŸ•— Version & Regression Information

⏯ Playground Link

https://www.typescriptlang.org/dev/bug-workbench/?target=7&ts=5.5.0-dev.20240512#code/PTAEAEGcBcCcEsDG0Bco4FcCmAoEEATLRAGwENYzp4B7AOzU13ygAsaB3AUQFt5o8YNp179oWAgDF4JLGjIA6AguiRBESO259o4qTLmgARkpVr14AGYG6ZHocWqcWAB4AHGrGih4dcbEsyRCxQAEk6UABvHFBQAG0AaywATzQYBDoAcwBdNAw6BLpOOgBuHABfZ3dPb19-QODQAHkMb2jYxJS0uF8cvIKijlKKqo8vdGS3EIAhMkgQgF5QAHIyZdAAHxWjZbKLa1lbezQTJ3geMbaJqbC6ABprkJboB+hJmbmQ8tBLWBoeUAAIgUwDIgL2rkuoEQ9BgoBoRgAVsRvEt2qACDQACqsXoAHixoFc4joBEgoFm8wAfAAKAD6qFAWIedN8aHCcSx2QAlGhnpzslEYrFQLAsNAMLBhrFyncKmUgA

πŸ’» Code

// @strict: true
// @declaration: true
// @showEmit
// @showEmittedFile: a.d.ts
// @showEmittedFile: b.d.ts

// @filename: a.ts
export interface In {
  [key: string]: unknown;
}
export interface Out {
  [key: string]: unknown;
}
export type Base = 'a' | 'b';

// @filename: b.ts
import { type In, type Out, type Base } from "./a";

export const object = {
  doThing<T extends Base>(_t: T, _in: In[T]): Out[T] {
    return;
  },
};

πŸ™ Actual behavior

When using the API, b.d.ts is emitted as

import { type In } from "./a";
export declare const object: {
    doThing<T extends Base>(_t: T, _in: In[T]): Out;
};

while when using tsc it gets emitted as

import { type In, type Out, type Base } from "./a";
export declare const object: {
    doThing<T extends Base>(_t: T, _in: In[T]): Out[T];
};

(interestingly the Debug tab of the workbench link shows something that's partially wrong)

πŸ™‚ Expected behavior

Using the API should emit

import { type In, type Out, type Base } from "./a";
export declare const object: {
    doThing<T extends Base>(_t: T, _in: In[T]): Out[T];
};

Additional information about the issue

No response

@MichaelMitchell-at
Copy link
Author

I'm somewhat worried that trying to adapt checker.ts to also be able to purely emit declarations will lead to more problems like these to continue to arise. It seems harder to reason about than the behavior we'd expect with isolated declarations - a mostly syntactic transformation. I'm concerned that these discrepancies will lead to compatibility issues when migrating to alternative implementations such as likely ones like ESBuild.

@weswigham weswigham added the Domain: ts.transpileDeclaration Issues regarding the transpileDeclaration API, which do not reproduce without it label May 13, 2024
@weswigham
Copy link
Member

This is partially fixed by #58522 - the Out is elided because we don't traverse the Out[T] node for reuse (and instead use an Out-printing error type node). The Base reference is a separate issue, so this gets to stay open - looks like we're failing to collect references from the type parameter constraint position in our references visitor (which is separate from the checker's node builder and largely standalone - the references visitor was always intended to be reused since import tracking is common functionality that all declaration emitters need to support elision).

@dragomirtitian
Copy link
Contributor

I'm concerned that these discrepancies will lead to compatibility issues when migrating to alternative implementations such as likely ones like ESBuild.

@MichaelMitchell-at We have the same concerns, and we are working to mitigate them.

While the errors for isolated declarations have been merged, we are still working on:

  1. Ensuring the TS emit is equivalent to what an external tool can reasonably emit
  2. Adding tests to ensure information in checker.ts is not accidentally being used during the emit of .d.ts files.

To elaborate on the second item, we are planning to extract a lot of the type reuse logic from checker.ts. This extracted type node builder (that will only use syntactic information) together with some other pieces we have already extracted (name resolution, evaluator) and other pieces we still need to extract (visibility checks) will be used to build a test declaration emitter that will not depend on checker.ts. This can be use to both prove that TS does not use any forbidden knowledge when emitting d.ts files in isolated declaration mode and it can serve as a blueprint for what other implementations need to reimplement in order to produce a similar emit (without needing to reason about the whole checker)

@MichaelMitchell-at
Copy link
Author

used to build a test declaration emitter that will not depend on checker.ts

Awesome! These changes continuing to be driven by folks at Bloomberg give us more confidence that isolated declarations will converge to be practical and impactful in large composite projects. At Airtable we're gradually migrating more projects to isolated declarations, which is helping to uncover bugs/edge cases and we'll continue to do so, though if there's anything we can contribute code-wise, we might be able to allocate some engineering time towards that. Please, let us know if there are any areas of opportunity for that. In particular, we'd be very interested in anything we can do to leverage isolated declarations to improve TSServer performance in VSCode. Editor performance has been the top pain point for our developers when it comes to our TypeScript usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: ts.transpileDeclaration Issues regarding the transpileDeclaration API, which do not reproduce without it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants