Add `transpileDeclaration` API method #58261

weswigham · 2024-04-19T23:16:35Z

With an identical signature to transpileModule, it differs only in that it fetches the .d.ts output for the given input, instead of .js, and in that it forces declaration, emitDeclarationOnly, and isolatedDeclarations on.

Do note, this is the... dummiest? simplest? possible version of this API - you can make a transpileDeclaration that actually loads the whole program graph and does not require isolatedDeclarations-compatible input to ensure reasonable output, but such a version is only practical (as far as most people's performance concerns go) once noCheck is merged, which will also help this and transpileModule avoid unnecessary work they're currently doing.

From the infrastructure side, of interest to most people working with isolatedDeclarations, this PR also offers a new transpile test runner, which runs compiler-like tests through the transpileModule and transpileDeclaration APIs (as appropriate for the options in the test) instead of the full compiler ones.

typescript-bot · 2024-04-19T23:16:46Z

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

jakebailey · 2024-04-19T23:23:17Z

src/testRunner/runner.ts

@@ -66,6 +67,8 @@ export function createRunner(kind: TestRunnerKind): RunnerBase {
            return new FourSlashRunner(FourSlash.FourSlashTestType.Server);
        case "project":
            return new project.ProjectRunner();
+        case "transpile":
+            return new TranspileRunner();


this is a mega good idea

When we have time, we should port the bulk of the existing transpile unit tests to this runner, probably. (The ones that don't pass in a custom transformer, at least.)

I also have a backlogged idea to make a runner for tscWatch-type tests, because the incredible boilerplate for those gets me every time I write one; just need to figure out a good way to encode the edits you usually want to apply in sequence into the input compiler-test-type fs description.

DanielRosenwasser · 2024-04-19T23:28:12Z

src/services/transpile.ts

+//  Late bound symbol names, in particular, are impossible to define without `Symbol` at least partially defined.
+// TODO: This should *probably* just load the full, real `lib` for the `target`.
+const barebonesLibContent = `/// <reference no-default-lib="true"/>
+interface Boolean {}


I suppose transpileDeclaration truly becomes unsafe to use without isolatedDeclarations because an inferred Boolean | Function will get reduced.

Without loading a full lib at least (which is doable! Just probably not worth if you're going to force on isolatedDeclarations anyway), yeah. Also your imports are all going to be unknown/error any.

Specifically, this barebones lib is the strategic mini-lib we use in our test harness to silence all global checker-construction errors, with the Symbol addenda so unique symbol types can actually be inferred.

It would be interesting to see what you end up with if you just add index signatures like [prop: string | symbol]: any

(probably don't do that in this PR though)

🤷‍♂️ even if the checker is recording the diagnostics when getEmitResolver is called right now, nothing goes and gets the semantic errors to report them. The emit might change if you add a test case with an inferred type that's sensitive to that, though.

weswigham · 2024-04-19T23:46:35Z

tests/baselines/reference/transpile/declarationBasicSyntax.d.ts

+==== class.ts (1 errors) ====
+    const i = Symbol();
+          ~
+!!! error TS9010: Variable must have an explicit type annotation with --isolatedDeclarations.


I'm sure this specific pattern (and the Symbol.for variant) is on the backlog of things to remove the isolatedDeclarations error from, right? Since unique symbols are so hard to use without const x = Symbol() allowing an inferred unique symbol type.

Yes; the problem is symbol freshness/widening. I tried my hand at fixing it but was unsuccessful. See also #55943 #55901

src/testRunner/transpileRunner.ts

DanielRosenwasser · 2024-04-19T23:46:30Z

src/services/transpile.ts

-    program.emit(/*targetSourceFile*/ undefined, /*writeFile*/ undefined, /*cancellationToken*/ undefined, /*emitOnlyDtsFiles*/ undefined, transpileOptions.transformers);
+    const result = program.emit(/*targetSourceFile*/ undefined, /*writeFile*/ undefined, /*cancellationToken*/ undefined, /*emitOnlyDtsFiles*/ declaration, transpileOptions.transformers, /*forceDtsEmit*/ declaration);
+
+    // TODO: Should this require `reportDiagnostics`?


Is it the case that the JS emit pipeline never reports errors?

Correct - emit pipeline errors were added for normal declaration emit when we ported them to transforms (and they add visibility errors very late) and are unused by js emit transforms -isolatedDeclarations continues to use the mechanism extensively. It's a longstanding issue I've wanted to fix, since it means we can't get declaration emit errors without actually running declaration emit (remind yourself how getPreEmitDiagnostics works some time 😑), but it's super annoying to try and decouple the errors from the node generation step.

fatcerberus · 2024-04-21T04:29:35Z

The name of transpileModule makes sense because it transpiles TS -> JS. transpileDeclarations is just nonsense, imo (extracting metadata is not usually considered "transpiling")

I don't have a suggestion for a better name though 😅

jakebailey · 2024-04-21T04:32:45Z

I don't have a suggestion for a better name though 😅

I feel like we also discussed just having it be transpileModule with a flag set such that the output file is a d.ts. But module is also a misnomer, as declaration emit (even with isolated declarations enabled) works on non-modules too!

jakebailey · 2024-04-22T17:13:41Z

src/services/transpile.ts

+interface Symbol {
+    readonly [Symbol.toStringTag]: string;
+}`;
+const barebonesLibName = "lib.d.ts";


Honestly this sorta weirds me out; is this something we should be formalizing in our real dts files somehow?

Eh, it's weirder that we can't typecheck unique symbol literals/calls without the skeleton of the global Symbol definitions present (since it's not like symbol types are ES6+ specific). That's probably the real underlying issue that needs fixing - we really aughta bake their presence into the checker like undefined if they're so central to checking a basic type-syntax construct. Though, there's probably some other annoying things, too, though - like I don't think we'll emit Omit types for generic spreads without the global Omit type present, which even this is currently missing. Eg,

export function f<T extends {x: number, y: number}>(x: T) { const {x: num, ...rest} = x; return rest; }

has an inferred return type of Omit<T, "x">, but without Omit in the lib, it's just any (the error kind). There are a lot of "builtins" like this, whose fallback behavior when they aren't present isn't particularly good.

Personally, by default I think I'd rather load the fully correct lib for the options passed in, at least for transpileDeclaration, just because I think emitting export const a: any for export const a = Math.random() feels bad (even if it is an error under ID), but I don't know if that runs counter to the perf goals of the API (even if every invocation with the same lib could share cached lib AST copies). Still, I guess it comes down to a question of scope - loading a real lib to get the builtins to work is easy, but should you need to do that for isolatedDeclarations-compatible input? I appended this barebones lib to the input because I assumed at least unique symbol-related inferences are planned to work, and this is the minimal lib (well, OK, I could omit the empty Array interfaces and the like) to get that.

We could leave it up to the caller - and load the default lib for the options provided unless an override (like this minimal one or a completely empty/missing one) is passed in, or load no lib at all by default unless one is passed in as a transpile option. It's really a question of the intended usage, I suppose.

jakebailey

Assuming that we are reporting diagnostics for isolatedDeclarations so people can't misuse the API (which in the design meeting you said was true), this all looks good to me as a start if we want to get feedback early for the beta and this makes it in. I do think that we need to figure out something better for the lib / symbols, but this seems okay to start.

weswigham · 2024-04-23T23:50:33Z

Assuming that we are reporting diagnostics for isolatedDeclarations

Yup, you can already see the errors in the .d.ts baselines, when emitted.

brandonmcconnell · 2024-04-24T08:30:25Z

Congrats, this is an exciting development!

Will this also support .d.cts, as checked by attw?

jakebailey · 2024-04-24T15:15:13Z

It should work with any TS file if it passes isolated declarations, but you're only going to get d.cts output from a .cts file input. Isolated declarations and transpileDeclaration don't really have anything to do with attw or module resolution.

sheetalkamat · 2024-04-24T16:54:56Z

src/services/transpile.ts


    if (transpileOptions.reportDiagnostics) {
        addRange(/*to*/ diagnostics, /*from*/ program.getSyntacticDiagnostics(sourceFile));
        addRange(/*to*/ diagnostics, /*from*/ program.getOptionsDiagnostics());
    }
    // Emit
-    program.emit(/*targetSourceFile*/ undefined, /*writeFile*/ undefined, /*cancellationToken*/ undefined, /*emitOnlyDtsFiles*/ undefined, transpileOptions.transformers);
+    const result = program.emit(/*targetSourceFile*/ undefined, /*writeFile*/ undefined, /*cancellationToken*/ undefined, /*emitOnlyDtsFiles*/ declaration, transpileOptions.transformers, /*forceDtsEmit*/ declaration);


I dont think we should set forceDtsEmit this one generates d.ts file even if declaration options are not set correctly and thats not what we are looking for right? It also generates output even if there are diagnostics. is that intentional.

Eg normally d.ts is not generated for json but this will generate it and i dont think we want that.

is that intentional.

Yes. transpileModule only fails (with a debug assert!) when the output isn't emitted at all, and will happily emit buggy output alongside diagnostics (if you ask for them). This keeps that behavior aligned between the two.

forceDtsEmit this one generates d.ts file even if declaration options are not set correctly and thats not what we are looking for right?

We set those options right above. We need to set forceDtsEmit so we still get output when the input has a parse error (because being able to return those errors is contingent on there being output at all).

Eg normally d.ts is not generated for json but this will generate it and i dont think we want that.

Honestly, if someone explicitly passes json into this API, it's because they wanted a declaration file for it.

JSON support sounds like a useful feature we might use. Our system treats JSON modules as frozen so potentially we could postprocess the output to express this readonly nature.

Just noting the differences between our normal emit vs setting forceDtsEmit which i had added for incremental checks

bigbigDreamer · 2024-07-15T16:07:44Z

Hi, I have a little confusion. Maybe it's not a problem. I hope someone can answer it. Thank you very much.

Here, since the transpileOptions are all optional parameters, why not directly set the second parameter as optional? This way, it won't lead to an error in the downstream when the upstream doesn't pass a parameter.

-function transpileDeclaration(input: string, transpileOptions: TranspileOptions): TranspileOutput;
+function transpileDeclaration(input: string, transpileOptions?: TranspileOptions): TranspileOutput;

The above is the problem I encountered when using transpileDeclaration. I'm not quite sure if the second parameter must be passed as an empty object. Maybe my question is a bit strange. If it bothers you, I'm deeply sorry. I'm just a little confused.

jakebailey · 2024-07-15T16:26:04Z

You most definitely are going to want to call this API with parameters, if only to ensure the compiler options are as expected. Notably strict is not the default, so if you don't pass in parameters, you will emit things like "undefined" incorrectly. An empty object is a bad idea for that reason.

weswigham added 5 commits April 17, 2024 15:21

Add transpileDeclaration API entrypoint

4e2febf

Small changes, API baseline update

425ffd0

New transpile runner for harness, transpile tests and a bugfix

26cae5b

Merge branch 'main' into transpile-declarations

94ab02d

Enable isolatedDeclarations, actually collect emit errors

18c6810

weswigham requested review from jakebailey and RyanCavanaugh April 19, 2024 23:16

typescript-bot assigned weswigham Apr 19, 2024

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 19, 2024

jakebailey reviewed Apr 19, 2024

View reviewed changes

DanielRosenwasser reviewed Apr 19, 2024

View reviewed changes

weswigham commented Apr 19, 2024

View reviewed changes

DanielRosenwasser approved these changes Apr 19, 2024

View reviewed changes

Lint feedback

b0a4b72

jakebailey reviewed Apr 22, 2024

View reviewed changes

weswigham requested a review from jakebailey April 23, 2024 22:07

weswigham assigned jakebailey Apr 23, 2024

jakebailey approved these changes Apr 23, 2024

View reviewed changes

weswigham added 2 commits April 23, 2024 16:52

Merge branch 'main' into transpile-declarations

6ee4762

Remove TODO comment, consensus is to keep these

764f861

weswigham merged commit 0b71b81 into microsoft:main Apr 24, 2024
25 checks passed

sheetalkamat reviewed Apr 24, 2024

View reviewed changes

DanielRosenwasser mentioned this pull request May 2, 2024

Design Meeting Notes, 4/23/2024 #58415

Open

This was referenced May 3, 2024

[transpileDeclaration API][5.5] Type containing enum values is incorrectly emitted #58423

Closed

[transpileDeclaration API][5.5] Type-only import is missing when type is used as arrow function argument #58430

Closed

This was referenced May 13, 2024

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

Closed

[transpileDeclaration API][5.5] Incorrectly creates new alias for generic parameter #58512

Closed

jakebailey mentioned this pull request Aug 15, 2024

transpileModule fails when declarationMap is specified #59641

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `transpileDeclaration` API method #58261

Add `transpileDeclaration` API method #58261

weswigham commented Apr 19, 2024

typescript-bot commented Apr 19, 2024

jakebailey Apr 19, 2024

weswigham Apr 19, 2024

DanielRosenwasser Apr 19, 2024 •

edited

Loading

weswigham Apr 19, 2024

weswigham Apr 19, 2024

DanielRosenwasser Apr 19, 2024

DanielRosenwasser Apr 19, 2024

weswigham Apr 20, 2024

weswigham Apr 19, 2024

jakebailey Apr 19, 2024

DanielRosenwasser Apr 19, 2024

weswigham Apr 19, 2024

fatcerberus commented Apr 21, 2024

jakebailey commented Apr 21, 2024

jakebailey Apr 22, 2024

weswigham Apr 22, 2024

jakebailey left a comment

weswigham commented Apr 23, 2024

brandonmcconnell commented Apr 24, 2024

jakebailey commented Apr 24, 2024

sheetalkamat Apr 24, 2024

weswigham Apr 24, 2024 •

edited

Loading

weswigham Apr 24, 2024

robpalme Apr 24, 2024

sheetalkamat Apr 24, 2024

bigbigDreamer commented Jul 15, 2024

jakebailey commented Jul 15, 2024 •

edited

Loading

Add transpileDeclaration API method #58261

Add transpileDeclaration API method #58261

Conversation

weswigham commented Apr 19, 2024

typescript-bot commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatcerberus commented Apr 21, 2024

jakebailey commented Apr 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey left a comment

Choose a reason for hiding this comment

weswigham commented Apr 23, 2024

brandonmcconnell commented Apr 24, 2024

jakebailey commented Apr 24, 2024

Choose a reason for hiding this comment

weswigham Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigbigDreamer commented Jul 15, 2024

jakebailey commented Jul 15, 2024 • edited Loading

Add `transpileDeclaration` API method #58261

Add `transpileDeclaration` API method #58261

DanielRosenwasser Apr 19, 2024 •

edited

Loading

weswigham Apr 24, 2024 •

edited

Loading

jakebailey commented Jul 15, 2024 •

edited

Loading