Get wct-mocha compiling clean. #755
Conversation
|
@rictic can you merge master into this? it should hopefully allow integration tests to pass. |
|
Also @rictic, this looks like a lot of work. Great job! |
|
Huh, this seems to be up to date with master. Maybe one of the changes I made wasn't as safe as I thought? |
|
Oh I assumed it was just failing because of Firefox 63, but you're right this branch does have the travis.yml pinning version to 62... |
|
I'll bisect the changes, will ping this thread again once it's ready to review :) |
This change causes the tests to fail!
rictic
force-pushed
the
wct-mocha-typings
branch
from
a86c8f0 to
9e0cb58
Compare
|
Error bisected and fixed, ready to review. Please take a look! |
|
Taking a look! |
| * @return A reporter-like "class" for each child suite | ||
| * that should be passed to `mocha.run`. | ||
| */ | ||
| childReporter(location: Location|string): ReporterFactory { | ||
| const name = this.suiteTitle(location); |
There was a problem hiding this comment.
So this was straight up not used? Huh. Good find.
There was a problem hiding this comment.
I just turned on warning for unused vars and props.
That's also why I made some properties not private, because they weren't used anywhere, but I wasn't confident enough to delete them, so they're public for now.
| if (target[key] !== null && typeof target[key] === 'object' && | ||
| !Array.isArray(target[key])) { | ||
| deepMerge(target[key], source[key]); | ||
| export function deepMerge<V extends {}>(target: V, source: V) { |
There was a problem hiding this comment.
😲I am learning some TypeScript from this PR.
There was a problem hiding this comment.
Yeah, I'd prefer to rewrite these deep merge functions (we have two!) into something less magical.. but that's for another time
|
Thanks for the quick review! |
Wanted to get this compiling clean as part of internalizing this