-
Notifications
You must be signed in to change notification settings - Fork 128
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want something that throws rather than console.assert, otherwise LGTM
let head = dom5.query(ast, dom5.predicates.hasTagName('head')); | ||
const ast = parse5.parse(contents); | ||
// parse5 always produces a <head> element | ||
const head = dom5.query(ast, dom5.predicates.hasTagName('head'))!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to const
ifying. Looks like typescript finds more and more places where they're able to do better type inference with const
over let
. (yes, const
-like behavior can ~always be inferred but the convenience of not having to do that does seem to be paying out for the tsc team) microsoft/TypeScript#10676
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to only touch declarations near other edits for this PR. We can do this incrementally or do a constification PR to follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, agree and affirm.
|
||
if (this.config.entrypoint && this.config.shell) { | ||
let file = this.fileMap.get(this.config.entrypoint); | ||
const file = this.fileMap.get(this.config.entrypoint)!; | ||
console.assert(file != null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this only logs, when I think you want to throw.
https://developer.mozilla.org/en-US/docs/Web/API/console/assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really want a short, real assertion. I'll make these a check + throw though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice, if I check null and throw, I don't need the !
type operator.
let deps = fragmentToDeps.get(im); | ||
for (const importUrl of this.config.allFragments) { | ||
const file = this.fileMap.get(importUrl)!; | ||
console.assert(file != null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -184,7 +184,8 @@ export class PolymerCli { | |||
|
|||
let commandName = parsedArgs.command; | |||
let commandArgs = parsedArgs.argv; | |||
let command = this.commands.get(commandName); | |||
let command = this.commands.get(commandName)!; | |||
console.assert(command != null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
PTAL |
No description provided.