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

TS JSDoc visibility error #41672

Open
hugomrdias opened this issue Nov 24, 2020 · 8 comments
Open

TS JSDoc visibility error #41672

hugomrdias opened this issue Nov 24, 2020 · 8 comments
Labels
Bug A bug in TypeScript
Milestone

Comments

@hugomrdias
Copy link

hugomrdias commented Nov 24, 2020

TypeScript Version: 4.1.2

Search Terms:
An explicit type annotation may unblock declaration emit
private name

Code

https://codesandbox.io/s/tsc-ts-docs-visibility-repro-f73gw?file=/src/index.js

// @filename: base.js
class Base {
    constructor() {}
}

const BaseFactory = () => {
    return new Base();
};

BaseFactory.Base = Base;

module.exports = BaseFactory;

// @filename: index.js
/** @typedef {import('./base')} BaseFactory */

/**
 *
 * @param {InstanceType<BaseFactory["Base"]>} base
 */
const test = (base) => {
    return base;
};

Expected behavior:
No error from the compiler

Actual behavior:

src/index.js:7:1 - error TS9006: Declaration emit for this file requires using private name 'Base' from module '"/sandbox/src/base"'. An explicit type annotation may unblock declaration emit.

7 const test = (base) => {
  ~~~~~


Found 1 error.

Playground Link:
https://codesandbox.io/s/tsc-ts-docs-visibility-repro-f73gw?file=/src/index.js

Related Issues:
#37832
#41250

@andrewbranch
Copy link
Member

Not sure whether @sandersn or @weswigham is the right person to take a look at this. I’m also not 100% sure if the example as given can compile, but the visibility error still appears when you annotate the declaration with /** @type {any} */, which definitely seems like it should be sufficient to satisfy the “An explicit type annotation may unblock declaration emit” error.

@weswigham
Copy link
Member

So, first caveat: You need an explicit @returns on that function as well (or the the function to not return the hard-to-get-at type of the parameter). Once you have that, you'd expect the error to go away (as now all appearances of the type are annotated), but it doesn't, because there's actually a bug in our serialization logic, where we don't reuse existing type nodes for jsdoc typedefs! Even though the error lands on the first statement (for reasons I have yet to look in to, errors don't go on comments very easily), the problem we run into is actually mostly when serializing the typedef! But wait, there's more! Once we start trying to reuse the input type node, something becomes apparent - typedef BaseFactory = import("./base") doesn't mean the same thing! Why? import("./base") has no type meaning. Based on usage of the resulting alias, what should have been written is /** @typedef {typeof import('./base')} BaseFactory */, but since there's no type meaning, you can omit the typeof in jsdoc and we still find "something" using our fallback jsdoc lookup behavior! However, since TS doesn't do that, then we can't reuse the input nodes, and we're back to the original case of being unable to reuse the input nodes, and unable to reference the type, and thus would issue this error!

So, rundown:
In your code, to allow this to print via declaration emit, you should:

  • Also include an @returns annotation
  • Write typeof import in the typedef rather than just import.

Those won't fix anything right now, but will in the future once in TS we:

  • Attempt to reuse input type nodes when serializing jsdoc typedefs, so when the above conditions are met, a typedef can successfully be produced and emitted (and issue an appropriate error if the above conditions aren't met) - as part of this, detection of when a bare import() gets jsdoc-fallback'd needs to get strengthened, as serializeExistingTypeNode only currently detects this for entity names that are part of type references, and not on import types. I've just opened JSDoc declaration emit should reuse input nodes where possible when serializing typedefs #41760 for this part.
  • Maybe clean up where errors get emitted in cases like this. Specifically, the error ends up on the first declaration because the typedef is attached to it - but a more specific span within the comment would probably be preferable here. I'm unsure if we should track that here, with this issue, or in a new issue.

@hugomrdias
Copy link
Author

where do you say i need @returns ? i think this is more about namespace imports from a @typedef than anything else.

Shouldn’t we be able to @typedef {import('base')} BaseFactoryNamespace and then @param {BaseFactoryNamespace.Base} ?

@weswigham
Copy link
Member

Nope - a typedef defines a new type, not an alias to all meanings (as a proper import does).

@sandersn
Copy link
Member

sandersn commented Jan 25, 2021

@hugomrdias Can you explain more about your scenario. WIth no other context, this seems like a rare usage of import('./base') compared to the normal usage const BaseFactory = require('./base'). Can you explain why you need to avoid commonjs-importing BaseFactory?

@weswigham Let's use this issue to track better emit errors on this comment, since you already fixed the node-reuse bug.

@sandersn
Copy link
Member

Later: The error is actually on the sourcefile, which causes it to be issued on the first statement, not necessarily the host statement of the typedef. The sourcefile is a fallback error node when none is returned from isSymbolAccessible.

@sandersn
Copy link
Member

#42501 improves the error span for jsdoc type aliases, which technically fixes this bug. However, there's more work to do on jsdoc tags that are not also type declarations, like @param, @return, @type, etc. I'm going to leave this bug open to track fixing the error span for those tags, but move it onto the backlog since I'm not sure it's a problem that exists in real code.

@Ethan-Arrowood
Copy link

I believe I'm hitting a similar error that also came up in #42405

In my case, I have a class that is not implementing the constructor of the class it extends from:

const { EventEmitter } = require('node:events');
class CustomEE extends EventEmitter { }
module.exports = CustomEE;

And this is causing a error TS9006: Declaration emit for this file requires using private name 'EventEmitterOptions' from module '"events"'. An explicit type annotation may unblock declaration emit.

When I update the constructor to:

class CustomEE extends EventEmitter {
  constructor(options) {
    super(options)
  }
}

the error goes away and the declaration compiles.

This behavior is unexpected because afaik it is valid to not implement the constructor method in JS.

I don't want to have to introduce extra JS code here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants