Skip to content

Improved 'nameless' JSDoc typedef support. #28162

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

Closed

Conversation

jameskeane
Copy link
Contributor

Being able to pass JSDoc @typedefs around as if they were values greatly reduces the need for a separate JSDoc import syntax (as in #22160 #26352 #22160). This commit improves upon the already existing nameless @typedef support, by:

  • Fixing a bug (JSDoc "nameless" @typedef on declaration does not work inside its own module #26626) in the binding of the type alias within external or CommonJS modules
  • Making nameless @typedefs have the same semantics as a native TypeAlias (i.e. type ex = number).
  • Adding error handling if trying to use a nameless (declared?) typedef as a value type, or with an initializer.
  • Adding a new conformance test case for the above mentioned features, and usage across modules.

Fixes #26626

 - Fixes microsoft#26626.
 - A nameless `@typedef` attached to a variable declaration will now have
   the same semantics as a native TypeAlias (i.e. `type ex = number`).
 - Added error handling if trying to use a nameless (declared?) typedef as
   a value type, or with an initializer.
 - Added new conformance test that showcases new features, and usage across
   modules.
@jameskeane
Copy link
Contributor Author

jameskeane commented Oct 26, 2018

@sandersn One thing I'd like to get working is assigning a @typedef to module.exports, but I'm realizing to do that I'd have to undo the changes I've made in the binder; what are your thoughts?

Also is fourslash documented anywhere? I'm kind of just poking in the dark with that syntax.

@sandersn sandersn self-assigned this Oct 29, 2018
@sandersn
Copy link
Member

I'll have to think more about this. My initial impression is that this doesn't match ES6 modules in structure (which is how I think of our current JSDoc support), or Closure typedefs in implementation (which is why we support nameless typedefs in the first place).

However, there is a need for @typedef to more closely match TS type aliases. It just weirds me out to see values treated only as types.

@jameskeane
Copy link
Contributor Author

It just weirds me out to see values treated only as types.

I agree. Initially I attempted to contextually treat it as a value or a type (only a value for es6 and commonjs exports), but it looks like it would be a more involved PR and require consensus.

You're right about the Closure implementation; I don't know why I just assumed that it reported an error if using a typedef as a value. Although, I can't think of a valid use-case for assigning a typedef, I'd be surprised if preventing it's use as value would present a compat issue.

btw, it looks like there's a big chunk of dead code in checkIdentifier; is that intentional? (checker.js line 15906, current master)

@sandersn
Copy link
Member

The discussion in #36375 is relevant here.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right direction for typedefs. The current solution, using import-type syntax in jsdoc, works fine but is a bit verbose. If we do anything else, I'd rather see an @import syntax.

But I think the current solution works fine if the usage is largely transitional JS to TS, which is what I believe is the case.

If transitioning a Closure codebase to Typescript would be easier with import support, that might change things. I'd be happy to see examples of that.

var this_is_not_the_name = true;

nameless = 123; // nameless is not a value
~~~~~~~~
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this error. We can't change the semantics of JS or TS such that nameless no longer has a value meaning.

/** @typedef {number} */
var notOK = 1;
~~~~~
!!! error TS2693: 'notOK' only refers to a type, but is being used as a value here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

export var type1;

==== tests/cases/conformance/jsdoc/c.js (0 errors) ====
import { type1 as aliased } from './b';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the other change of this PR, I think. I'm not sure I like this one either -- it encourages checkJS users to import and export real variables at runtime just for the purposes of creating aliases at compile time. There's no general guarantee that these imports and exports will get optimised away.

@sandersn
Copy link
Member

This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here.

@sandersn sandersn closed this Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

JSDoc "nameless" @typedef on declaration does not work inside its own module
2 participants