🏗🐛 Remove JSDoc from third party code before compiling it (reprise) #21878

rsimha · 2019-04-17T01:58:25Z

This PR removes JSDoc comments from code in third_party/ by disguising all instances of /** as /*#.

As a result:

The comments are no longer treated as JSDoc
They are no longer used by closure compiler to perform compilation optimizations around inlining, devirtualization, minification, etc.
The code size remains unchanged because of equal length substitution between /** and /*#
Source maps remain unaffected (instead of being offset)

rsimha · 2019-04-17T02:00:54Z

@dvoytenko As a result of this change, we will now see several type errors in amp-subscriptions-google and amp-subscriptions. Let me know how you'd like me to fix them. The good news is that these should be one-time fixes :)

Type errors

[10:53:12] gulp-google-closure-compiler: extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js:90: ERROR - Function ConfiguredRuntime$$module$third_party$subscriptions_project$swg: called with 3 argument(s). Function requires at least 4 argument(s) and no more than 4 argument(s).
    this.runtime_ = new ConfiguredRuntime(
                    ^^^^^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js:359: ERROR - can only implement interfaces
class AmpFetcher {
^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js:364: ERROR - can only implement interfaces
  constructor(win) {
  ^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js:370: ERROR - property fetchCredentialedJson not defined on any superclass of AmpFetcher$$module$extensions$amp_subscriptions_google$0_1$amp_subscriptions_google
  fetchCredentialedJson(url) {
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js:371: ERROR - inconsistent return type
found   : Promise<?>
required: undefined
    return this.xhr_.fetchJson(url, {
           ^^^^^^^^^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:25: ERROR - can only implement interfaces
export class DocImpl {
       ^^^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:30: ERROR - can only implement interfaces
  constructor(ampdoc) {
  ^^^^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:36: ERROR - property getWin not defined on any superclass of DocImpl$$module$extensions$amp_subscriptions$0_1$doc_impl
  getWin() {
  ^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:37: ERROR - inconsistent return type
found   : Window
required: undefined
    return this.ampdoc_.win;
           ^^^^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:41: ERROR - property getRootNode not defined on any superclass of DocImpl$$module$extensions$amp_subscriptions$0_1$doc_impl
  getRootNode() {
  ^^^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:42: ERROR - inconsistent return type
found   : (Document|ShadowRoot)
required: undefined
    return this.ampdoc_.getRootNode();
           ^^^^^^^^^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:46: ERROR - property getRootElement not defined on any superclass of DocImpl$$module$extensions$amp_subscriptions$0_1$doc_impl
  getRootElement() {
  ^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:48: ERROR - inconsistent return type
found   : Element
required: undefined
    return dev().assertElement(root.documentElement || root.body || root);
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:52: ERROR - property getHead not defined on any superclass of DocImpl$$module$extensions$amp_subscriptions$0_1$doc_impl
  getHead() {
  ^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:53: ERROR - inconsistent return type
found   : Element
required: undefined
    return dev().assertElement(this.ampdoc_.getHeadNode());
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:57: ERROR - property getBody not defined on any superclass of DocImpl$$module$extensions$amp_subscriptions$0_1$doc_impl
  getBody() {
  ^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:58: ERROR - inconsistent return type
found   : (Element|null)
required: undefined
    return this.ampdoc_.isBodyAvailable() ? this.ampdoc_.getBody() : null;
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:62: ERROR - property isReady not defined on any superclass of DocImpl$$module$extensions$amp_subscriptions$0_1$doc_impl
  isReady() {
  ^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:63: ERROR - inconsistent return type
found   : boolean
required: undefined
    return this.ampdoc_.isReady();
           ^^^^^^^^^^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:67: ERROR - property whenReady not defined on any superclass of DocImpl$$module$extensions$amp_subscriptions$0_1$doc_impl
  whenReady() {
  ^^^^^^^^^^^^^

extensions/amp-subscriptions/0.1/doc-impl.js:68: ERROR - inconsistent return type
found   : Promise
required: undefined
    return this.ampdoc_.whenReady();
           ^^^^^^^^^^^^^^^^^^^^^^^^

21 error(s), 0 warning(s), 91.4% typed

build-system/tasks/compile.js

rsimha · 2019-04-17T03:05:08Z

I'll hold off against merging this until we have a good strategy for fixing type errors in extensions code that uses third party code. See #21878 (comment)

/cc @choumx @erwinmombay @kristoferbaxter @cramforce for visibility

dreamofabear · 2019-04-17T08:47:40Z

IMO it seems like a shame to nuke all typing in swg.js due to one or two missing type paths. This will increase risk of type errors across swg.js and its callers.

dvoytenko · 2019-04-18T08:38:37Z

@choumx TBH, any typecheck inside swg.js will not be actionable for AMP. This is somewhat similar to WorkerDOM case - we basically have types inside and mostly lose them when we import it in AMP. I think what could really help if we could define the library-exported types, similar to .d.ts in typescript. But I don't think Closure allows anything like this...

dreamofabear · 2019-04-19T07:37:10Z

@dvoytenko Do we ignore types when calling swg.js public APIs from amphtml? Also, amp-subscriptions-google.js an interface class from swg.js.

amphtml/extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js

Lines 18 to 22 in b765e57

    
           import { 
        
             ConfiguredRuntime, 
        
             Fetcher, 
        
             SubscribeResponse, 
        
           } from '../../../third_party/subscriptions-project/swg';

amphtml/third_party/subscriptions-project/swg.js

Lines 7475 to 7478 in b765e57

    
           /** 
        
            * @interface 
        
            */ 
        
           class Fetcher {

rsimha · 2019-04-22T05:15:08Z

@dvoytenko As asked by @choumx, would we like to start ignoring all type info in swg.js? Assuming the answer is yes, can you comment on the lines below, and whether they should be removed?

amphtml/extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js

Lines 398 to 420 in b765e57

    
           /** 
        
            * TODO(dvoytenko): remove once compiler type checking is fixed for third_party. 
        
            * @package @visibleForTesting 
        
            */ 
        
           export function getFetcherClassForTesting() { 
        
             return Fetcher; 
        
           } 
        
           /** 
        
            * TODO(dvoytenko): remove once compiler type checking is fixed for third_party. 
        
            * @package @visibleForTesting 
        
            */ 
        
           export function getPageConfigClassForTesting() { 
        
             return PageConfig; 
        
           } 
        
           /** 
        
            * TODO(dvoytenko): remove once compiler type checking is fixed for third_party. 
        
            * @package @visibleForTesting 
        
            */ 
        
           export function getSubscribeResponseClassForTesting() { 
        
             return SubscribeResponse; 
        
           }

dvoytenko · 2019-04-23T00:39:06Z

@rsimha I'd like to start mostly ignoring types in third_party modules as they are most likely not currently correctly interpreted. However, we need a solution of how to explain typing for the types that AMP specifically depends on, similar to .d.ts in TypeScript. Especially tricky are interfaces that 3p libraries want AMP to implement. Out of possibly dozens of types in swg.js only about 3-4 are needed for AMP. Is there anything of the kind?

rsimha · 2019-04-23T19:02:40Z

@dvoytenko I found this article that might be helpful: https://medium.com/@trukrs/type-safe-javascript-with-jsdoc-7a2a63209b76. A couple of the solutions involve installing a VS Code plugin that does inline type checking while you're coding. Not sure if there's something in there that can be done as a check during CI. Let me know what you think.

Also, let me know how you'd like to proceed with this PR. Does it make sense to merge it as is (remove all types from all third party code)? If so, how would you like the errors in amp-subscriptions-google and amp-subscriptions to be fixed?

dvoytenko · 2019-04-23T23:12:03Z

@rsimha I don't think that article applies since we need integration on our build pipeline, not vscode. But w/o solving these issues we cannot submit this pull request. Unless we have some form of d.ts-like type definitions for closure, we may have to go back to doing one-off changes to jsdocs to avoid errors and take some risks with that.

rsimha · 2019-04-24T03:42:25Z

I'm still unclear on what the check is supposed to do. Is the goal to remove type info from 3p source code so closure doesn't complain, and then use some other mechanism to infer the type info and make sure it's correct?

dvoytenko · 2019-04-24T19:06:48Z

@rsimha My ideal scenarios are either one of the following:

Rollup somehow learns how to inline all types in a 3p project. So all types are correct. We'd still need to disable errors/warnings in 3p projects, but we will actually be somewhat able to use this type information.
A more correct approach, however, is to ignore type information in a 3p project itself (replacement or any other way) but instead provide library-export typing separately as a type.d.ts-style of file. This would work for both: closure and non-closure 3p imports. And this reflects the likely scenario that out of all the types/classes/functions/etc that a 3p project might be using, the exported surface is much smaller.

Specifically, currently the main problem is with interfaces, which is a concept that's otherwise absent from JavaScript itself. Unless we have a good solution for this (d.ts or similar) I think we'll have to keep the file and do on-off fixes on broken types.

rsimha · 2019-05-21T23:13:50Z

After discussing offline with @dvoytenko, there's no pressing need to do something as drastic as removing all type annotations from swg.js.

Part of the reason behind this discussion was #21827 (review), which turned out to be a red herring due to an unnecessary global find-replace I did while upgrading closure compiler.

For now, we are fine with unknown types in swg.js because we ignore errors that stem from them:

amphtml/build-system/compile/compile.js

Lines 92 to 95 in 2b9201c

    
           const hideWarningsFor = [ 
        
             'third_party/caja/', 
        
             'third_party/closure-library/sha384-generated.js', 
        
             'third_party/subscriptions-project/',

This PR can be abandoned.

rsimha requested review from jridgewell and dvoytenko April 17, 2019 01:58

rsimha self-assigned this Apr 17, 2019

googlebot added the cla: yes label Apr 17, 2019

rsimha mentioned this pull request Apr 17, 2019

third_party/ code must not be treated as closure code #21828

Closed

jridgewell approved these changes Apr 17, 2019

View reviewed changes

build-system/tasks/compile.js Outdated Show resolved Hide resolved

rsimha added Blocked WG: infra labels Apr 17, 2019

Remove JSDoc from third party code before closure-compiling it

81b7f52

rsimha closed this May 21, 2019

rsimha deleted the 2019-04-17-Remove3pJsDoc branch May 21, 2019 23:13

rsimha mentioned this pull request May 30, 2019

Improve type checking across 3p modules #22585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏗🐛 Remove JSDoc from third party code before compiling it (reprise) #21878

🏗🐛 Remove JSDoc from third party code before compiling it (reprise) #21878

rsimha commented Apr 17, 2019

rsimha commented Apr 17, 2019 •

edited

Loading

rsimha commented Apr 17, 2019

dreamofabear commented Apr 17, 2019

dvoytenko commented Apr 18, 2019

dreamofabear commented Apr 19, 2019

rsimha commented Apr 22, 2019

dvoytenko commented Apr 23, 2019

rsimha commented Apr 23, 2019

dvoytenko commented Apr 23, 2019

rsimha commented Apr 24, 2019

dvoytenko commented Apr 24, 2019

rsimha commented May 21, 2019

🏗🐛 Remove JSDoc from third party code before compiling it (reprise) #21878

🏗🐛 Remove JSDoc from third party code before compiling it (reprise) #21878

Conversation

rsimha commented Apr 17, 2019

rsimha commented Apr 17, 2019 • edited Loading

rsimha commented Apr 17, 2019

dreamofabear commented Apr 17, 2019

dvoytenko commented Apr 18, 2019

dreamofabear commented Apr 19, 2019

rsimha commented Apr 22, 2019

dvoytenko commented Apr 23, 2019

rsimha commented Apr 23, 2019

dvoytenko commented Apr 23, 2019

rsimha commented Apr 24, 2019

dvoytenko commented Apr 24, 2019

rsimha commented May 21, 2019

rsimha commented Apr 17, 2019 •

edited

Loading