Amphtml: Type paths to missing files in swg.js

Created on 6 Oct 2018  路  6Comments  路  Source: ampproject/amphtml

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

https://github.com/ampproject/amphtml/blob/6b112a74dbab7a6269fae43d3b041a5829448cc7/third_party/subscriptions-project/swg.js#L2272-L2278

!../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: https://github.com/ampproject/amphtml/pull/18552

/to @jpettit /cc @dvoytenko @dparikh

Soon Bug access-subscriptions

Most helpful comment

@jpettitt

All 6 comments

@jpettitt

/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.

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

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.

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: https://github.com/google/closure-compiler/issues/3125

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

Was this page helpful?
0 / 5 - 0 ratings

Related issues

Download picture Download  路  3Comments

mkhatib picture mkhatib  路  3Comments

akshaylive picture akshaylive  路  3Comments

cvializ picture cvializ  路  3Comments

mrjoro picture mrjoro  路  3Comments