microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.08k stars 12.49k forks source link

In declaration files generated from JavaScript files and their JSDoc, unresolved types are not converted to `any`. #47025

Open tamuratak opened 2 years ago

tamuratak commented 2 years ago

Bug Report

With TypeScript 4.5.2, when we generate declaration files from JavaScript files and their JSDoc annotations with tsc --allowJs --declaration --emitDeclarationOnly, unresolved types are not converted to any. I can see the same issue with 4.6.0-dev.20211204.

🔎 Search Terms

JSDoc, allowJs, declaration

🕗 Version & Regression Information

💻 Code

Generate main.d.ts from main.js with tsc --allowJs --declaration --emitDeclarationOnly main.js

main.js

/**
 * 
 * @param {SomeClass} a 
 * @returns {SomeClass}
 * 
 */
function f(a) {
    return a
}

🙁 Actual behavior

With 4.6.0-dev.20211204, main.d.ts :

/**
 *
 * @param {SomeClass} a
 * @returns {SomeClass}
 *
 */
declare function f(a: SomeClass): SomeClass;

🙂 Expected behavior

With 4.4.4, main.d.ts:

/**
 *
 * @param {SomeClass} a
 * @returns {SomeClass}
 *
 */
declare function f(a: any): any;
RyanCavanaugh commented 2 years ago

Seems like we have two conflicting principles at work here:

I'm not sure --declaration --allowJs really makes sense without also specifying --checkJs -- there are any number of invalid declaration files you can create under this scheme. Depending on TS to catch those and fall back to any is not going to be waterproof.

tamuratak commented 2 years ago

On PDF.js's side, only --declaration --allowJs is used to generate *.d.ts files:

npx tsc --allowJs --declaration --outDir build/types/  --emitDeclarationOnly src/pdf.js web/pdf_viewer.component.js

You can see the generated files:

The generated *.d.ts files are actually very useful for PDF.js's users.

When I call with --checkJs, I see 749 errors reported. I don't think adding annotations to fix these errors is feasible.

there are any number of invalid declaration files you can create under this scheme.

We had better say non-strict rather than invalid. I think TypeScript users know how to live with non-strict declaration files anyway.

frank-dspeed commented 2 years ago

@tamuratak i am also a havy .js only typescript user and i think the current behavior is correct to keep the type declarations strict.

when you get invalid types we need to fix that on a other end i am also working on tooling to package .d.ts files so that you could ship complet working types even when you package your code at the end.

that is possible because .d.ts files can overlap so it does not hurt in normal cases.

Proposal

I would accept a --noneStrictDts setting that allows to convert unresolved to any but existing behavior should not change what do you think?

noahtallen commented 1 year ago

This is somewhat unrelated to any issue above, but basically the same context with a project using a ton of existing JSDoc.

On PDF.js's side, only --declaration --allowJs is used to generate *.d.ts files:

We're in a similar situation over in the Gutenberg project (https://github.com/WordPress/gutenberg). While we are incorporating Typescript more and more, we have to rely on the jsdoc -> typescript workflow for many of the existing packages, some of which are quite large.

While I'd love to fix every type issue that exists in the project, we mostly want to be able to publish types for users asap, and the vast majority of JSDoc types would still make this a decent experience.

I'm not sure --declaration --allowJs really makes sense without also specifying --checkJs -- there are any number of invalid declaration files you can create under this scheme.

The main issue we run into is that with checkJs: true, we can see sometimes many thousands of internal type errors. (Mostly because JSDoc isn't as flexible as TS.) If we disable this, though, a very small subset of those errors could be published to the *.d.ts files.

In one example, just one error out of many got included into the .d.ts file. Ideally, I would like tsc to output just that error for us to fix -- this way, we can still publish valid .d.ts files for consumers while we slowly migrate more things to Typescript.

RyanCavanaugh commented 1 year ago

@noahtallen it seems like "build with tsc --allowJs --declaration followed by seeing if tsc output.d.ts builds without error" fully accomplishes what you're asking for?

noahtallen commented 1 year ago

Thanks for chiming in! That does work 👍 I think it will be pretty easy to integrate in various workflows as well.