Fix all type errors in v0.js #4908
Conversation
New status. 99 error(s), 6 warning(s), 93.87904883060965% typed
|
@dvoytenko Could you check the changes to resources, fixed-layer and url-replacements? |
| return dev().assert(Resource.forElementOptional(element), | ||
| 'Missing resource prop on %s', element); | ||
| return /** @type {!Resource} */ ( | ||
| dev().assert(Resource.forElementOptional(element), |
There was a problem hiding this comment.
We don't get ! from assert?
There was a problem hiding this comment.
Bug in our CC implementation. Will fix with @erwinmombay
There was a problem hiding this comment.
I'm pretty sure this used to work, but regressed with the dev() changes
There was a problem hiding this comment.
yeah type inference system for asserts dont work with dev().assert(). no quick solution yet
There was a problem hiding this comment.
Type inference seems to be having issues in general. Not sure exactly what
is going on.
On Fri, Sep 9, 2016 at 4:29 PM, erwin mombay notifications@github.com
wrote:
In src/service/resource.js
#4908 (comment):@@ -74,8 +74,9 @@ export class Resource {
* @return {!Resource}
*/
static forElement(element) {
- return dev().assert(Resource.forElementOptional(element),
'Missing resource prop on %s', element);- return /** @type {!Resource} */ (
dev().assert(Resource.forElementOptional(element),yeah type inference system for asserts dont work with dev().assert(). no
quick solution yet—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/pull/4908/files/9dc6e433e9326d421095feb7f85465681e43f296#r78263228,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT3b9_Zxya-0DqG_4Qwgon8hre5vVks5qoevSgaJpZM4J5hjb
.
0 error(s), 21 warning(s), 94.03821780247635% typed
|
LGTM on my side. |
- Fix lint - Fix tests
|
@jridgewell Had to turn off SW compilation. Will look into it. |
- Turn off compilation of sw due to warnings.
|
Have a green build in https://travis-ci.org/ampproject/amphtml/builds/158915579 and will take @dvoytenko's LGTM Others, please take a look. In particular for stuff that I didn't do perfectly since this had to get in quickly, so we have type checking on travis. |
| @@ -98,6 +98,7 @@ protected AmpCommandLineRunner(String[] args) { | |||
| protected CompilerOptions createTypeCheckingOptions() { | |||
| CompilerOptions options = super.createOptions(); | |||
| options.setCheckTypes(true); | |||
| options.setInferTypes(true); | |||
There was a problem hiding this comment.
same, tested w/ and w/o and didn't see any difference. (test done with your PR)
Fixed all errors and made all relevant warnings errors and fixed them all :)
Turns on type checking for v0.js on travis.