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

Support for opting out of jsdoc @typedef exports #46011

Open
5 tasks done
jespertheend opened this issue Sep 22, 2021 · 40 comments
Open
5 tasks done

Support for opting out of jsdoc @typedef exports #46011

jespertheend opened this issue Sep 22, 2021 · 40 comments
Labels
In Discussion Not yet reached consensus Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@jespertheend
Copy link
Contributor

jespertheend commented Sep 22, 2021

Suggestion

πŸ” Search Terms

jsdoc typedef export #43207 #23692

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ 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 andrewbranch added In Discussion Not yet reached consensus Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Sep 23, 2021
@andrewbranch
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@brendankenny
Copy link
Contributor

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 google/closure-compiler#3041 it's clear this can lead to the same issues that lead to import type in TypeScript)

@jespertheend
Copy link
Contributor Author

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

@jespertheend
Copy link
Contributor Author

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 #22160 (comment)

@kungfooman
Copy link

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
Copy link

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

#23723

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

@ljharb
Copy link
Contributor

ljharb commented Oct 5, 2023

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
Copy link
Member

@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
Copy link
Contributor

ljharb commented Oct 5, 2023

@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
Copy link
Member

Opened #56002

@sandersn
Copy link
Member

sandersn commented Oct 9, 2023

@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
Copy link

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 πŸ˜…

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
Copy link
Contributor Author

What about a completely new tag? Something like

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

It would

  • seem closer to a 'default local' since no extra tag is required
  • be easier to parse, since @typedefs and @localtypes can coexist along side each other in the same comment
  • not break existing code

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
Copy link
Member

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
Copy link
Contributor Author

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:

  • A new separate tag. Something like @local @typedef, or maybe @private @typedef.
  • A singular tag. Something like @localtype.

@kungfooman
Copy link

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
Copy link
Contributor Author

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
Copy link
Contributor

ljharb commented Oct 27, 2023

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link

kungfooman commented Oct 28, 2023

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: #46011 (comment)

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
Copy link
Contributor

ljharb commented Oct 28, 2023

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
Copy link
Contributor Author

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
Copy link

stoicbuddha commented May 28, 2024

@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
Copy link

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

@stoicbuddha
Copy link

@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
Copy link
Contributor

ljharb commented May 29, 2024

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

@stoicbuddha
Copy link

@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
Copy link
Contributor

ljharb commented May 29, 2024

@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
Copy link

@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
Copy link
Contributor

ljharb commented May 29, 2024

@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
Copy link

@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
Copy link
Contributor

ljharb commented May 29, 2024

@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
Copy link

@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
Copy link
Contributor

ljharb commented May 29, 2024

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
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants