type annotations are duplicated between typescript syntax and jsdoc

[alexeagle]

It seems like you have a lot of extra maintenance burden to have types in the jsdoc that match the types declared in TypeScript:

  /**
   * @param {Observer|function(value: T): void} [destinationOrNext] A partially
   * defined Observer or a `next` callback function.
   * @param {function(e: ?any): void} [error] The `error` callback of an
   * Observer.
   * @param {function(): void} [complete] The `complete` callback of an
   * Observer.
   */
  constructor(destinationOrNext?: PartialObserver<any> | ((value: T) => void),
              error?: (e?: any) => void,
              complete?: () => void) {

is there any benefit to having the types in JSDoc?

[benlesh]

I was literally just in a meeting about this because there is a project at Netflix that is using Closure compiler and this type information is breaking it, because it's invalid for closure compiler.

The only real benefit here is for the documentation.

[benlesh]

1. It's going to suck to go through and change all of this for limited gain.
2. If we change it, it arguably hurts the docs readability, but maybe not?

[alexeagle]

What generates the documentation? We use typescript language services to extract type info into the docs. Probably not hard to fix, right?

[alexeagle]

oh, @blesh has your Netflix project seen https://github.com/angular/tsickle ?

[alexeagle]

I dug in a bit further, following the threads about how there is no TSDoc, ESDoc requires types in the JSDoc position, and no tool to lower the type information into comments in the emit.

However, Tsickle does do this (github.com/angular/tsickle) and I tried building rxjs/dist/es6 with tsickle's typed mode, and then run esdoc on the result. It fails at the first site of an unknown type (cc @martine):

error: could not process the following code.
/Users/alexeagle/Projects/RxJS/dist/es6/AsyncSubject.js
21|     /**
22|      * @param {?} value
23|      * @return {void}
24|      */
25|     _next(value) {

/Users/alexeagle/Projects/RxJS/node_modules/esdoc/out/src/ESDoc.js:409
            throw _iteratorError5;
            ^

Error: Empty Type found name=value desc=
    at Function.parseParam (/Users/alexeagle/Projects/RxJS/node_modules/esdoc/out/src/Parser/ParamParser.js:159:15)

I filed https://github.com/esdoc/esdoc/issues/282 for that.

But even if ESDoc accepts it, also tsickle produces closure compiler compatible ES6 which includes a place to give types to properties declared as constructor params:

class MapOperator {
    /**
     * @param {function(?, number): ?} project
     * @param {?} thisArg
     */
    constructor(project, thisArg) {
        this.project = project;
        this.thisArg = thisArg;
    }
    /**
     * @param {Subscriber<?>} subscriber
     * @param {?} source
     * @return {?}
     */
    call(subscriber, source) {
        return source._subscribe(new MapSubscriber(subscriber, this.project, this.thisArg));
    }
    static _tsickle_typeAnnotationsHelper() {
        /** @type {function(?, number): ?} */
        MapOperator.prototype.project;
        /** @type {?} */
        MapOperator.prototype.thisArg;
    }
}

and I suspect these static members then appear in ESDoc, unless there's a configuration or esdoc plugin to strip members with leading underscore.

Angular generates docs directly from TS sources with a long, convoluted path I would not recommend for anyone :)

It seems like tsickle is a promising option for generating docs with ESDoc that include types declared in the TS file, and have type annotations that work for non-TS consumers.

[jayphelps]

Just a minor note since it didn't seem to be elaborated:

Generating the docs just from the types alone misses out on all the other important doc like full param descriptions, example code, as well as just just the general comment block.

e.g. https://github.com/ReactiveX/rxjs/blob/master/src/observable/ArrayObservable.ts#L27-L63

There's a buttload of stuff there that if it wasn't there would have to be somewhere else and if it's not inline it's far more likely to be out of date/sync IMO. Ideally someone would create a tool that verifies the param types you document are in fact the same type/count as the TS signature.

[typhonrt]

@jayphelps Most of what you linked to looks like valid JSDoc regarding #L27-L63, so ESDoc shouldn't complain in that regard. I made a few notes in the linked ESDoc #282 issue that a potential new ESDoc plugin hook for parsing parameters with the proper plugin hook could handle TS or Closure Compiler typing.

I've been writing a lot of plugins for ESDoc and I hope I can convince the sole owner to add and expand more plugin hooks or get added as a collaborator as the last thing I want to do right now is fork ESDoc. In all of my framework / architecture efforts I stick to straight ES6 and ESDoc is serving well right now for the most part..

[jayphelps]

Sorry, I wasn't meaning to imply that was an example of incorrect JSDoc or similar. The discussion initially hinted at removing inline doc comments altogether and I just wanted to slip in a very strong opinion that they stay. The other discussion I lack context on.

[typhonrt]

I kind of jumped in here too through the ESDoc side of the portal regarding context though have an eye on utilizing rxjs in the near future. I can confirm the opinion that inline documentation is great. I work w/ JSPM / SystemJS and I've got an ESDoc plugin that links all of the JSPM managed code including inheritance across modules that has valid ESDoc data (NPM version soon), another one that completely replaces the left hand navigation for an enhanced JSPM aware version (click JSPM Managed source to reveal / also context click to jump to linked code on Github) and even D3 interactive dependency graphs (JSPM packages for now / context click on nodes to jump to NPM or Github repo!). This is for any ad hoc relationship JSPM can load. Soon I'll have source code and file graphs too for JSPM then NPM ES6 modules. I created a fork of Backbone / backbone-es6 to modernize it and specifically to provide inline docs for end to end documentation. I've stuck to pure ES6, but getting ESDoc to accept TS / Closure Compiler typing would be neat.

[staltz]

ESDoc isn't the best tool for this library, that's true. We had to use the least worst tool for the job. If it comes to maintenance, I'm ok to manually tailor JSDoc to match TypeScript, if it gets the job done.

[alexeagle]

Sorry there was never a proposal to remove the non-type info from JSDoc. Of course any solution, eg. TSDoc, would have to fold the type information into the JSDoc documentation.

The presence of these types actually breaks some closure tooling, both in my case and some other project @blesh mentions, because these are not correct Closure type annotations. I'm working around it for now with a local modification in one of the tools. For example, here's a snippet from dist/global/Rx.umd.js which is the input for npm run build_closure_core - Closure reports these type annotations are wrong (what does any mean if you're not in a .ts file??)

http://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F%252F%2520ADD%2520YOUR%2520CODE%2520HERE%250Avar%2520Notification%2520%253D%2520(function%2520()%2520%257B%250A%2520%2520%2520%2520function%2520Notification(kind%252C%2520value%252C%2520exception)%2520%257B%250A%2520%2520%2520%2520%2520%2520%2520%2520this.kind%2520%253D%2520kind%253B%250A%2520%2520%2520%2520%2520%2520%2520%2520this.value%2520%253D%2520value%253B%250A%2520%2520%2520%2520%2520%2520%2520%2520this.exception%2520%253D%2520exception%253B%250A%2520%2520%2520%2520%2520%2520%2520%2520this.hasValue%2520%253D%2520kind%2520%253D%253D%253D%2520'N'%253B%250A%2520%2520%2520%2520%257D%250A%2520%2520%2520%2520%252F**%250A%2520%2520%2520%2520%2520*%2520Given%2520some%2520%257B%2540link%2520Observer%257D%2520callbacks%252C%2520deliver%2520the%2520value%2520represented%2520by%2520the%250A%2520%2520%2520%2520%2520*%2520current%2520Notification%2520to%2520the%2520correctly%2520corresponding%2520callback.%250A%2520%2520%2520%2520%2520*%2520%2540param%2520%257Bfunction(value%253A%2520T)%253A%2520void%257D%2520next%2520An%2520Observer%2520%2560next%2560%2520callback.%250A%2520%2520%2520%2520%2520*%2520%2540param%2520%257Bfunction(err%253A%2520any)%253A%2520void%257D%2520%255Berror%255D%2520An%2520Observer%2520%2560error%2560%2520callback.%250A%2520%2520%2520%2520%2520*%2520%2540param%2520%257Bfunction()%253A%2520void%257D%2520%255Bcomplete%255D%2520An%2520Observer%2520%2560complete%2560%2520callback.%250A%2520%2520%2520%2520%2520*%2520%2540return%2520%257Bany%257D%250A%2520%2520%2520%2520%2520*%252F%250A%2520%2520%2520%2520Notification.prototype.do%2520%253D%2520function%2520(next%252C%2520error%252C%2520complete)%2520%257B%250A%2520%2520%2520%2520%2520%2520%2520%2520var%2520kind%2520%253D%2520this.kind%253B%250A%2520%2520%2520%2520%2520%2520%2520%2520switch%2520(kind)%2520%257B%250A%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520case%2520'N'%253A%250A%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520return%2520next%2520%2526%2526%2520next(this.value)%253B%250A%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520case%2520'E'%253A%250A%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520return%2520error%2520%2526%2526%2520error(this.exception)%253B%250A%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520case%2520'C'%253A%250A%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520%2520return%2520complete%2520%2526%2526%2520complete()%253B%250A%2520%2520%2520%2520%2520%2520%2520%2520%257D%250A%2520%2520%2520%2520%257D%253B%250A%257D())%253B

On Fri, May 13, 2016 at 1:53 AM André Staltz notifications@github.com wrote:

ESDoc isn't the best tool for this library, that's true. We had to use the least worst tool for the job. If it comes to maintenance, I'm ok to manually tailor JSDoc to match TypeScript, if it gets the job done.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/ReactiveX/rxjs/issues/1703#issuecomment-218987536

[benlesh]

I think we still need to go through and validate that all JSDoc comments are valid for Closure compiler and are not TypeScript types.

[typhonrt]

Just a FYI. I'm soon releasing in a week the spiritual successor to ESDoc called TJSDoc. Unfortunately the ESDoc developer didn't want to accept outside contributions or discuss much at all, so TJSDoc was born.

Since the beginning of the year hundreds of hours of work has gone into providing an even better architecture that internally is all plugin based supporting multiple runtimes with a common core. Currently the Babylon runtime is nearly complete. I'm just finishing up a new template system based on material-components-web. Up next is the Typescript runtime!

I'll be making the initial major commit of TJSDoc before the GDC conference (in a week) and release on NPM, but if there is any interest certainly please follow the refactoring post here as I'll update that when the release goes live.

I would be honored to work with the rxjs community creating the best Typescript support for TJSDoc that exceeds all documentation concerns for rxjs. The issue that @blesh just mentioned JSDoc comments not being Typescript types is something that technically would vanish as the types will be parsed implicitly for doc generation and or explicit types in documentation can be parsed as Typescript types.

[kylecordes]

@benlesh Yes - the TS source JSDoc has lots of (for example):

 * @param {Observer|function} [nextOrObserver] A normal Observer object or a

... which Closure Compiler objects to - it wants at least a function() in there, and really the full type of that function.

How about something in the RxJS build process that tries to Closure-compiler it, and emits whatever warnings/errors emerge?

[typhonrt]

From my understanding of the first two comments in this issue adding the typing to the @param comment is just for ESDoc to pick up the type info for doc generation. Potentially moving forward with TJSDoc in a couple of months this type information can be automatically populated for doc generation. I'm curious what things could look like at that time and what if anything needs to be present for the Closure compiler.

IE would @param {*} [nextOrObserver] A normal Observer object or a... work as TJSDoc will replace the * with the AST parsed type information for doc generation?

By default for the Typescript variant of TJSDoc or when Flow typing is enabled for ES6+ / Babylon my general idea is that it will override any types manually specified in the docs. Does the Closure compiler need specific type information present as well or will the wildcard suffice?

[kylecordes]

@typhonrt I'm not enough of a Closure Compiler guru to know exactly what it needs. However, it reads these comments originally aimed at JSDoc, and does not like what it sees, and emits numerous warnings.

By the way, I assume you've read the rest of this thread and elsewhere, the Angular team's tsickle seems at least adjacent and what sort of things it does compared to your upcoming TJSDoc. I wonder if there would be a way to do only the additional work needed on top of what it already does.

[benlesh]

Closing as stale

[typhonrt]

Just a FYI. I took a break from open source work, but will be back on TJSDoc soon to finish it off and get it launched w/ Babylon 7 support which should include Typescript. I'm also angling to support TSDoc doc comment tags which is still being defined as a Typescript alternative to JSDoc. Probably will have a fall official release; it'll be a nice documentation tool! One of the killer features I've added is live doc regeneration; start the doc tool and a browser opens w/ current docs and regenerates them quickly on the fly as new source code or comments are added to a project. I'll ping the RxJS team when it's got a full vetted release.