Don't reject files with invalid @jsdoc

[evmar]

In angular/angular#8550 they modified tsickle to add an "ignoreTypesInComments" flag:

This prevents tsickle from throwing an error when it encounters jsdoc in comments. This is needed to compile RxJS which has a bunch of jsdoc comments.

In the Google build, we disable tsickle errors when building third-party code, which is a lame hack. We should instead make tsickle configurable, to either reject or silently fix up this code.

[evmar]

@jjudd this should be sufficient to implement the "don't die on warnings" feature you wanted.

At Google the policy typically is: either make the message fatal or don't show it. So for our purposes we'll make these warnings fatal in first-party code and ignore warnings in third-party code.

[kylecordes]

@evmar What would you think about merging that code Lucid mentioned that adds the ignoreTypesInComments flag? I've been doing the same as the Google examples, disabling warnings from Closure Compiler because it emits hundreds of them for RxJS which contains a bunch of JSDoc type comments.

I think tsickle will be more useful if it is able to consume (accidentally) "hostile" code, code that has a bunch of human-entered JSDoc types. Such code is essentially guaranteed statistically to have incorrect types. In fact this makes me wonder if ignoreTypesInComments should be the default, always on behavior rather than an option. (That may be wrong though, there may be certain cases where passing through incoming types is useful.)

[evmar]

There are two kinds of code:

• code you control, where you might change the comments; tsickle warns "don't put types in comments in TypeScript because they're not useful"
• code you don't control like rxjs, where it's not useful for tsickle to yell at you because you're not going to change it

We model this in tsickle by always stripping illegal comments (such as jsdoc types) and emitting a warning when we do. So I think accidentally hostile code should already work? It's only a question of which messages are printed.

[kylecordes]

@evmar In some work earlier today I was unable to reproduce your second bullet point. tsickle appeared to be happily passing through "param" jsdoc annotations which then generated warnings from Closure Compiler - because upon inspection they did not meet CC requirements, they were not generated by tsickle, they were equally present in the source rxjs files. Am I missing something maybe? I could try adding a test case that verifies an invalid JSdoc param type marker doesn't make it through.

[evmar]

The test case we have is here:

tsickle/test_files/jsdoc/jsdoc.ts

Line 17 in a481172

* @param {badTypeHere} foo no types allowed.

(the other files in that directory are the tsickle output; the .tsickle.ts shows the tsickle warnings)

But clearly we got something wrong if this is affecting you. Can you give some more details that might help us figure out what's going wrong, or reduce it to a smaller test case?

[kylecordes]

@evmar Thank you, I'm going to study that and try to reproduce the thing affecting me as a test case or similar. I'll report back with details assuming I find something to report. (I'm trying to get out of the habit of just complaining on bug trackers and into the habit of crafting a test case or occasionally a PR to nail down a problem and move to a solution instead... )

[evmar]

I appreciate you taking the effort to file a bug! We are still working on the open source story for tsickle. Unfortunately using it typically requires integration into your build and our build system within Google is pretty different from the standard web ones. I suspect the error you're encountering has something to do with that.

[kylecordes]

I now believe that the case I saw was not a reproduction of this bug. Rather, I was looking over at @alexeagle 's work on Closure Compiler with Angular. Neither tsconfig there:

https://github.com/alexeagle/closure-compiler-angular-bundling/blob/master/vendor/tsconfig.rxjs-tsickle.json

Nor the rxjs baseline tsconfig specifies module:commonjs, a hint. I'm unable to verify that it actually performs tsickle processing - the output does not appear to contain any tsickle-generated enhanced JSDoc comment.

That code there runs tsc-wrapped (which appears to be designed to call tsickle alongside other Angular specific processing for ngc). I hypothesize that it does this because by running this way it is possible to specify the file name of a tsconfig, while the tsickle CLI cannot currently do so.

The actual problem I was working on to pick this up, was to run tsickle successfully on RxJS source. Doing so generates a great number of warnings/errors, I will continue iterating toward that goal and open new issues if they come up - probably not this issue.

[alexeagle]

It turns out that we don't need to run tsickle on rxjs source to make Angular work with Closure. In that demo repo, take a look at the package.json:
https://github.com/alexeagle/closure-compiler-angular-bundling/blob/master/package.json#L25
I experimented with three ways of building rxjs, all of them work. The one I use on master is just vanilla tsc to compile rxjs.
Of course the intent is to get rxjs to distribute a package that just works, then we're not involved in compiling it.

[kylecordes]

@alexeagle Hi... I'm a big fan of your linked repo. I have spent a number of hours with (a fork of) it. As far as I can tell (from much careful looking!) it does not actually tsickle RxJS. The 3 different ways are really just 2 different ways. The one labeled "tsickle" does not actual do any tsickling, at least I can't make it do so. If it did, it would run in to the thing I hit in #420.

(My goal: a PR at RxJS to make it emit Closure-ready distributables.)

[alexeagle]

`@angular/tsc-wrapped` is a wrapper around `tsc` that runs tsickle. The line `"annotateForClosureCompiler": true` in the tsickle tsconfig is an option for tsickle, so it should be modifying the jsdocs.

…

On Tue, Mar 7, 2017 at 12:32 PM Kyle Cordes ***@***.***> wrote: @alexeagle <https://github.com/alexeagle> Hi... I'm a big fan of your linked repo. I have spent a number of hours with (a fork of) it. As far as I can tell (from much careful looking!) it does *not* actually tsickle RxJS. The 3 different ways are really just 2 different ways. The one labeled "tsickle" does not actual do any tsickling, at least I can't make it do so. If it did, it would run in to the thing I hit in #420 <#420>. (My goal: a PR at RxJS to make it emit Closure-ready distributables.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#177 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC5I9D7Nminec2Hp3nUl45uSQ4-B0NEks5rjb7VgaJpZM4JTDej> .

[kylecordes]

@alexeagle Yes - that matches my understanding. But I was not able to make it do so. I looked at its output, and saw no goog.___ nor generated JSDocs, just the original incoming RxJS JSDocs and ES2015 imports and exports.