microsoft / TypeScript

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

Support for opting out of jsdoc @typedef exports #46011

Open jespertheend opened 3 years ago

jespertheend commented 3 years ago

Suggestion

🔍 Search Terms

jsdoc typedef export #43207 #23692

✅ Viability Checklist

My suggestion meets these guidelines:

⭐ Suggestion

Currently when using /** @typedef {Number} Num */ the type Num is automatically exported when the comment is in the root scope. I'd like to prevent this from happening.

📃 Motivating Example 💻 Use Cases

My problem: FileA.js declares /** @typedef {Number} Num */ FileB.js uses this type a bunch of times, so instead of using /** @type {import("./FileA.js").Num} */ everywhere, it declares it once at the top of the file: /** @typedef {import("./FileA.js").Num} Num */ and then uses Num everywhere in the file.

The problem is that doing it this way in all files creates a lot of exports of the same type and it's not always clear which one is the original export. This creates a rather big list with Intellisense: image

A woraround is to wrap the type in parentheses, this causes the typedef to be limited to that scope, but this is not always feasible. I.e:

{
    /** @typedef {import("./FileA.js").Num} Num */
    /**
     * @param {Num}
     */
    export default function foo(x) { // <-- error, export needs to be top level

    }
}
andrewbranch commented 3 years ago

Agreed this is not ideal. Not sure how best to indicate that it shouldn’t be exported. Maybe another JSDoc tag on the comment:

/**
 * @typedef {import("./FileA.js").Num} Num
 * @local
 */

@local isn’t something listed on https://jsdoc.app. @private or something? We kind of try not to just make up JSDoc things willy nilly, but this feels like a problem we made so the solution will probably have to be made by us too. /cc @sandersn?

jespertheend commented 3 years ago

I'm guessing this would no longer be an issue if #22160 is fixed. That would allow you to import Num without exporting it.

sandersn commented 3 years ago

I think the closest tag is @module.

jsdoc.app and Closure must have the problem to some degree since they have the concept of scope. What do they do? Are @typedef exportable somehow? I'd like to know how those two systems handle it before changing ours.

jespertheend commented 3 years ago

It seems JSDoc.app is having a similar issue https://github.com/jsdoc/jsdoc/issues/1537 I think both JSDoc.app and Closure are both exporting @typedef to file scope just like TypeScript is currently doing.

brendankenny commented 3 years ago

jsdoc.app and Closure must have the problem to some degree since they have the concept of scope. What do they do? Are @typedef exportable somehow? I'd like to know how those two systems handle it before changing ours.

For Closure Compiler, at least, typedefs are attached to a real value-space declaration. For example:

/** @typedef {(string|Element|Text|Array<Element>|Array<Text>)} */
goog.ElementContent;

Types in the Closure Type System - Typedefs

(it's been a few years since I regularly wrote Closure-able code, but I don't believe anything has changed)

As a result, export/import for typedefs is explicit and the same as bringing in any dependency (though from https://github.com/google/closure-compiler/issues/3041 it's clear this can lead to the same issues that lead to import type in TypeScript)

jespertheend commented 3 years ago

I suppose this might be a duplicate of #22160 At least that would solve the same issue.

jespertheend commented 1 year ago

Best workaround I've found for this so far is to assign a type to an unused variable, and then use typeof variable whenever you need to use the type:

/**
 * @type {{
 *  foo: number,
 *  bar: string,
 * }}
 */
let myType;

/**
 * @param {typeof myType} x
 */
function takesMyType(x) {

}

The downside of this that it creates an unused variable, so you might have to disable your linter every time you do this.

Other than that I'm frequently importing types within the closure of a class or function, and I'm using tons and tons of inline type imports, which results in some truly horrendous looking code as can be seen in https://github.com/microsoft/TypeScript/issues/22160#issuecomment-1404399423

kungfooman commented 1 year ago

Problem:

image

Even though it's just this @typedef in each file (to make it nicer...):

/**
 * @typedef {import('./utils/hub.js').PretrainedOptions} PretrainedOptions
 */

Solution should be as simple as implementing @private for @typedef's to not auto-export everything blindly?

kungfooman commented 1 year ago

I think I found the problematic PR: first @typedef's were local and now they are all exported:

https://github.com/microsoft/TypeScript/pull/23723

@sandersn Did you have this export * from ...-in-entry-point-index-file scenario in mind when dealing with that PR?

ljharb commented 1 year ago

I'm running into this now; I have a CJS file with JSDoc comments that's generating types, and the resulting types have export = (as they should) for the CJS value, and also export type for the typedef, despite not explicitly exporting it - which causes tsc to error.

How can I define a type purely in jsdoc and not export it?

andrewbranch commented 1 year ago

@ljharb that actually sounds like a declaration emit bug separate from this feature request. You should be getting an export = of a namespace containing the type, merged with your module.exports object, like this: https://www.typescriptlang.org/play?module=1&filetype=js#code/PQKhAIAEBcE8AcCmATRAzcBvAztATgJYB2A5gL7gBiA9teCMAFCNoCuRAxtAdUeGgAoAlFjLMAttWSsANogB0iAB7xqeaNnABefgG4gA

While being able to avoid exporting a typedef would work around this (and I still think it would be a good feature), you’re supposed to be able to use module.exports and still export types without emitting invalid declarations.

Can you show how to repro the bug you’re seeing?

ljharb commented 1 year ago

@andrewbranch ah, fair point. in that case, https://github.com/ljharb/set-function-name/tree/tsc, npm run emit-types && npm run tsc should repro the failure.

andrewbranch commented 1 year ago

Opened #56002

sandersn commented 1 year ago

@sandersn Did you have this export * from ...-in-entry-point-index-file scenario in mind when dealing with that PR?

No, definitely not.

i like the idea of a modifier like @local or @private or @module on @typedef, although I'd like it go in a place that doesn't make @typedef much more complicated to parse.

It's unfortunately probably too late to change the semantics to require something like @export on @typedef for it to be exported from modules.

kungfooman commented 1 year ago

It's unfortunately probably too late to change the semantics to require something like @export on @typedef for it to be exported from modules.

Yea, your PR is @export'ing them since 2018, lets see both cases:

1) default @export... this is against "local by default" which is the standard in ESM world.

2) default @local... this may break some projects which started to rely on the behaviour since 2018.

I would rather aim for (2) to keep the ESM no-exporting-unless-stated behaviour "aligned", but based on legacy, we cannot simply change it. So the only option I see is to... make it an option :sweat_smile:

So once we have an option, we need to define how to export a @typedef:

/**
 * @typedef {object} A
 * @export
 * @typedef {object} B
 */

Is this exporting A or B? IMO a very specific naming is causing the least ambiguity:

/**
 * @typedef {object} A
 * @typedef {object} B
 * @typedef {object} C
 * @export A, C - Everyone should understand that B is not exported here?
 */

The default option can for my sake be (1) - keep defaulting to exporting typedefs. I've seen several projects by now being affected by this, so if the fix comes in the form of an option: problem solved.

I wonder why @DanielRosenwasser gave a thumbs-down on your 2018-typedef-export PR, maybe because of this issue? Lets just discuss a bit, decide on a solution and go for it?

jespertheend commented 1 year ago

What about a completely new tag? Something like

/**
 * @localtype {object} A
 */

It would

Not sure about the name of the tag exactly, ideally it would look similar to @typedef but shorter somehow, to convey the idea that this new tag is the default, whereas @typedef seems more like an extension of that new tag.

An other idea could be to add two completely new tags and deprecate the @typedef tag, but I'm not sure how feasible that is. I don't think any JSDoc tags have ever been deprecated in TypeScript, let alone one as common as @typedef.

sandersn commented 1 year ago

I like @localtype because it's easy to specify and easy to parse (at least, no harder than `@typedef). But for users it's not easy to discover and mysterious if you run across it in random code.

@export, on the other hand, implies that it mirrors all of JS exports, which is quite complex -- see @kungfooman 's example of both a separate @export A, C and an export modifier @export @typedef .... It is, probably, the right solution in the abstract, though, since we try to have jsdoc mirror normal TS/JS constructs as much as possible.

We're definitely not able to deprecate anything because I expect a lot of JSDoc's value is in using VS Code (or even VS!) to open ancient loose .js files and get some kind of help from jsdoc comments.

jespertheend commented 1 year ago

One problem with @export though, is that it is not really clear how to prevent any type from being exported. Specifically, if you have a JSDoc comment with only a single @typedef.

Something like

/**
 * @typedef {number} Foo
 * @export _
 */

to indicate that no type should be exported from that block comment could work. But honestly, that seems even more mysterious than @localtype at that point.

If we don't want to deprecate anything, then @export is pretty much out of the question.

So at this point the only options I see are:

kungfooman commented 1 year ago

One problem with @export though, is that it is not really clear how to prevent any type from being exported.

That's a simple condition:

const str = `
/**
 * @typedef {number} Foo
 * @export _, 123, Foo
 */
function add(a, b) {
  return a + b;
}`;
const ast = ts.createSourceFile('repl.ts', str, ts.ScriptTarget.Latest, false /*setParentNodes*/);
const {tags} = ast.statements[0].jsDoc[0];
for (const tag of tags) {
    if (tag.tagName.text === 'export') {
        const names = tag.comment.split(',').map(_ => _.trim());
        for (const name of names) {
            const typedef = tags.find(tag => tag.kind === ts.SyntaxKind.JSDocTypedefTag && tag.name.text === name);
            if (!typedef) {
                console.log(`Trying to find @typedef called ${name}, but can't find it.`);
                continue;
            }
            console.log("Exporting", typedef.name.text);
        }
    }
}

Output:

image

We're definitely not able to deprecate anything because I expect a lot of JSDoc's value is in using VS Code (or even VS!) to open ancient loose .js files and get some kind of help from jsdoc comments.

Yes, solved by making it an option, maybe strictJSDoc: true? @localtype sounds very strange to me anyway and it also goes against everything ESM tries to fix (keeping things local except they are exported) to not cause any unexpected issues... exactly the kind of issues we are running into here.

@export, on the other hand, implies that it mirrors all of JS exports, which is quite complex -- see @kungfooman 's example of both a separate @export A, C and an export modifier @export @typedef .... It is, probably, the right solution in the abstract, though, since we try to have jsdoc mirror normal TS/JS constructs as much as possible.

Since JSDoc is about types, isn't it "implies that it mirrors all of TS exports"? In the beginning we can just limit it to typedef's in the same JSDoc as a "minimal working solution" and see what other ideas developers have.

Idea for even more syntax: @export(typedef) ... (that would bind the export as much as possible to the typedef)

jespertheend commented 1 year ago

Yes, solved by making it an option, maybe strictJSDoc: true?

That works, but only if all your libraries are up to date and expect that option to be set. If one of your libraries doesn't have any of its types @exported and you want to use some of its types, you'll be out of luck.

I agree @localtype feels pretty weird, and so do @local @typedef or @private @typedef. @export could work, but it will be another // @ts-check that you have to add to every file and I'd like to avoid that where possible.

I'm thinking of what it would be like to migrate an existing project, and adding a tag to every file just to opt out of type exporting is not really something I'd want to do. You might forget to set it in one file and accidentally export all your types without being aware of it.

ljharb commented 1 year ago

If one of your libraries doesn't have any of its types @exported and you want to use some of its types, you'll be out of luck.

this is true of anything, and that’s fine. If you want access to something that’s not exported, you either ask the maintainer, fork it, or you don’t get access

jespertheend commented 1 year ago

this is true of anything

But it doesn't have to be. A new option would be very close to just deprecating exports. And to quote sandersn:

We're definitely not able to deprecate anything because I expect a lot of JSDoc's value is in using VS Code (or even VS!) to open ancient loose .js files and get some kind of help from jsdoc comments.

Projects with this new option enabled would lose this value just as much as if the behaviour was deprecated.

Don't get me wrong, an option is better than not fixing this issue at all. I just think there are better alternatives :)

Take JavaScripts var for instance: spec authors realised that var was not strict enough. So instead of adding yet another "use strict"; kind of statement at the top of files, they came up with let and const. A new JSDoc tag would have the same benefits as this approach. Legacy code can still use @typedef while modern code uses the new tag.

kungfooman commented 1 year ago

Take JavaScripts var for instance: spec authors realised that var was not strict enough. So instead of adding yet another "use strict"; kind of statement at the top of files, they came up with let and const.

Please note that they have also come to realize that certain aspects are not viable and as a result, they are now causing syntax errors, for example:

"use strict";
var static = 1;

image

IMO we are in the same situation here - ESM is inherently about limiting scope. If you want to make something available outside the module, you must explicitly export it using the export keyword.

I think we don't even need strictJSDoc: true, we can simply decide via package.json and type === 'module'. Because ESM implies "local by default".

jespertheend commented 1 year ago

I think we don't even need strictJSDoc: true, we can simply decide via package.json and type === 'module'. Because ESM implies "local by default".

I'm using Deno and don't have any package.json.

If a new JSDoc tag is not something we are happy with (though I still think it's the best option, maybe it just needs a better name than @localtype), then I propose the following:

A new @export tag is used to mark types as exported. Files without any @export tags will export all types, like they have always done. But any file that contains at least one @export tag will only export the specified types. This approach is similar to how TypeScript currently detects module files:

   await Promise.resolve();
// ^^^^^--- 'await' expressions are only allowed at the top level of a file when that file is a module,
// but this file has no imports or exports. Consider adding an empty 'export {}' to make this file a module.

Doing it like this wouldn't break legacy code, allowing you to gradually update your codebase, old libraries would keep working, and you wouldn't need to configure anything.

The only downside is that this might be somewhat unexpected, but an error message similar to the 'top level await' one could take care of that.

kungfooman commented 1 year ago

A new @export tag is used to mark types as exported. Files without any @export tags will export all types, like they have always done.

Then a file using this style still doesn't work: https://github.com/microsoft/TypeScript/issues/46011#issuecomment-1538383663

Or how could that be detected? Your idea is nice and I like it and we just make a special case for export * from ... and if any of those files export a @typedef, then we are following ESM guidelines?

Edit: (Now I kinda have a fear that people will just export useless dummies simply because there is no good option)

ljharb commented 1 year ago

I think we don't even need strictJSDoc: true, we can simply decide via package.json and type === 'module'. Because ESM implies "local by default".

this would preclude CJS packages from having the benefit, which is my entire use case.

jespertheend commented 1 year ago

I would say the use of export * from ... shouldn't have any influence of the @typedefs inside that file. I.e. @typedefs from foo.js would still get exported when export * from "./foo.js" is used in bar.js, whether or not bar.js has an @exports tag.

Edit: (Now I kinda have a fear that people will just export useless dummies simply because there is no good option)

That's why I prefer a new JSDoc tag :)

stoicbuddha commented 6 months ago

@import doesn't re-export the types, ex:

@import {MyControllerType} from "../controllers/my"

This works in VS Code for me as well, although I think it's currently only available in the Nightly build. You can also use it to import multiple typedefs from the same file, ex:

@import {MyControllerType, SomeOtherControllerType} from "../controllers/my"

kungfooman commented 6 months ago

@stoicbuddha I see the value of @import, but how does it help with this @typedef export * from ... issue?

stoicbuddha commented 6 months ago

@kungfooman As far as I can tell, it solves the original issue, which is that @typedef exports anything you import with it. @import can be used to avoid that problem. I could be wrong, but I think export * from ... is a different issue? I may just be misunderstanding it.

ljharb commented 6 months ago

@stoicbuddha the issue is that @typedef can't be used to define a non-exported type.

stoicbuddha commented 6 months ago

@ljharb Based on my understanding of the OP, the issue is that using @typedef to import a type, ex:

@typedef {import("./types").SomeType} SomeType

then re-exports the type from that file.

Using @import works the same way but does not re-export in this manner, which solves that problem. IMO, @typedef should export, as you're defining the type (even if it's via import), while @import should not, as it's just an import.

I'm having the exact same issue as the OP regarding a bunch of exports from within my project because of @typedef which is how I discovered that @import exists.

ljharb commented 6 months ago

@stoicbuddha that's true if you are importing as part of the typedef. but typedefs aren't just for imports - @typedef {{ foo: number }} FooType eg also would export FooType, and that's undesirable.

It's great to know that @import can replace that one use case of typedef, but there's many others :-)

stoicbuddha commented 6 months ago

@ljharb In that instance, wouldn't it make more sense to either:

A) Use @type on the value you need to type, ex: @type {{foo: number}}?

B) Add @typedef {{ foo: number }} FooType to a separate types file and use @import to bring it in?

ljharb commented 6 months ago

@stoicbuddha for A, no, because I want to reuse the defined type in multiple places, and for B no, because i don't want it accessible from outside the file, and if it's importable, then it's accessible anywhere.

stoicbuddha commented 6 months ago

@ljharb I'm confused as to why the accessibility is an issue. Can you clarify the use case in which you define a type that is specific to code in a given file that gets imported somewhere else? I'm guessing you have some experience with that where I don't, and I'm not trying to discount it, I'm just not clear on why that would be happening.

ljharb commented 6 months ago

@stoicbuddha it's a type i don't want directly usable outside the file. The use case is "i don't want the name of the type to be part of my semver-compliant public API". It's the same reason you wouldn't want every variable in a module automatically become exported :-)

stoicbuddha commented 6 months ago

@ljharb I understand what the effects are, but I think we'd agree that a type and a variable are wildly different levels of dangerous in this context; a variable is actually usable, whereas a type is just evaluated by the TS compiler and has no real bearing on the code itself.

What I'm trying to ascertain is: what is the real-world instance where you have a @typedef in a separate file, with a type specific to that file, which is imported for use elsewhere by yourself or another developer unintentionally?

ljharb commented 6 months ago

Given editor autocompletion, it happens a lot. Additionally, in a published package, if i rename an exported type it's a semver-major change - the level of danger there is precisely as dangerous as for a variable because it can fail CI pipelines.

stoicbuddha commented 6 months ago

@ljharb I could see editor autocompletion pollution being a use case for having it be specific to a file. It still seems like you'd need to intentionally use it, but the pollution would be annoying.

Regarding the danger, I think we are talking about two different things (and perhaps have a disagreement as to what we'd regard as dangerous), but it's probably not important for solving the overall issue.