-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Add transpileDeclaration API method
#58261
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
Conversation
|
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. |
| case "project": | ||
| return new project.ProjectRunner(); | ||
| case "transpile": | ||
| return new TranspileRunner(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a mega good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| // 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 {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose transpileDeclaration truly becomes unsafe to use without isolatedDeclarations because an inferred Boolean | Function will get reduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be interesting to see what you end up with if you just add index signatures like [prop: string | symbol]: any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(probably don't do that in this PR though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️ 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.
| ==== class.ts (1 errors) ==== | ||
| const i = Symbol(); | ||
| ~ | ||
| !!! error TS9010: Variable must have an explicit type annotation with --isolatedDeclarations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/services/transpile.ts
Outdated
| 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`? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it the case that the JS emit pipeline never reports errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
The name of I don't have a suggestion for a better name though 😅 |
I feel like we also discussed just having it be |
| interface Symbol { | ||
| readonly [Symbol.toStringTag]: string; | ||
| }`; | ||
| const barebonesLibName = "lib.d.ts"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly this sorta weirds me out; is this something we should be formalizing in our real dts files somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Yup, you can already see the errors in the |
|
Congrats, this is an exciting development! Will this also support
|
|
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. |
| } | ||
| // 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting the differences between our normal emit vs setting forceDtsEmit which i had added for incremental checks
|
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. |



With an identical signature to
transpileModule, it differs only in that it fetches the.d.tsoutput for the given input, instead of.js, and in that it forcesdeclaration,emitDeclarationOnly, andisolatedDeclarationson.Do note, this is the... dummiest? simplest? possible version of this API - you can make a
transpileDeclarationthat actually loads the whole program graph and does not requireisolatedDeclarations-compatible input to ensure reasonable output, but such a version is only practical (as far as most people's performance concerns go) oncenoCheckis merged, which will also help this andtranspileModuleavoid unnecessary work they're currently doing.From the infrastructure side, of interest to most people working with
isolatedDeclarations, this PR also offers a newtranspiletest runner, which runscompiler-like tests through thetranspileModuleandtranspileDeclarationAPIs (as appropriate for the options in the test) instead of the full compiler ones.