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

Type paths to missing files in swg.js #18586

Closed
dreamofabear opened this issue Oct 6, 2018 · 6 comments
Closed

Type paths to missing files in swg.js #18586

dreamofabear opened this issue Oct 6, 2018 · 6 comments

Comments

@dreamofabear
Copy link

dreamofabear commented Oct 6, 2018

New versions of Closure Compiler upgraded "module not found" from warnings to errors. For example:

/**
* @param {!../model/doc.Doc} doc
*/
constructor(doc) {
/** @private @const {!../model/doc.Doc} */
this.doc_ = doc;
}

!../model/doc.Doc doesn't exist in the bundled SWG binary.

Unfortunately don't seem suppressible with @suppress {moduleLoad}, individually or at the @fileoverview level. The current workaround is to not fail the build on warnings in swg.js replace these types with * via regex (see #18589).

To fix this, we can either strip these bogus types, use externs, or bundle these missing classes.

Context: #18552

/to @jpettit /cc @dvoytenko @dparikh

@dreamofabear dreamofabear changed the title Fix type paths to missing files in swg.js Type paths to missing files in swg.js Oct 7, 2018
@jpettit
Copy link

jpettit commented Oct 8, 2018

@jpettitt

@dvoytenko
Copy link
Contributor

/cc @erwinmombay I believe our decision was to keep comments in 3p as is, but ignore them - i.e. consider these files as not closure at all.

@jpettitt
Copy link
Contributor

@choumx @erwinmombay can we close this based the comment from @dvoytenko ?

@dreamofabear
Copy link
Author

Discussed with Dima and we agreed that type annotations in swg.js should be ignored. This means either (1) swg.js starts stripping JSDoc in its output or (2) amphtml strips JSDoc from swg.js in a compile preprocess.

This issue should be left open to track the implementation of one of these two.

@dvoytenko
Copy link
Contributor

Currently SwG strips out types on the way. I'm not super happy about it, but it works for now. I do anticipate problems with this in the future and I believe (2) is the better path forward. But for now we have (1).

I also filed this: google/closure-compiler#3125

@jpettitt jpettitt removed their assignment Nov 20, 2018
@dreamofabear
Copy link
Author

Closing as status quo is acceptable by both AMP and SWG.

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

No branches or pull requests

5 participants