Enable strictBindCallApply. #2074
Conversation
tsconfig.base.json enables "strict" now which implicitly enables "strictFunctionTypes", "strictNullChecks", and "noImplicitAny" so we don't need to do so explicitly. Disabling "alwaysStrict" and "noImplicitThis" was unnecessary since we have no violations.
| ): PersistencePromise<void>; | ||
| static forEach<R, S>( | ||
| collection: { forEach: (cb: (r: R, s?: S) => void) => void }, | ||
| f: (r: R, s?: S) => PersistencePromise<void> |
There was a problem hiding this comment.
I split this into two overloads. One for collections with keys and values, one for collections with just keys. Without this, you get:
Argument of type 'S | undefined' is not assignable to parameter of type 'S'. Type 'undefined' is not assignable to type 'S'.
from the f.call(this, r, s) line below (for s being undefined).
| ); | ||
| this.delayedOperations.push(delayedOp); | ||
| this.delayedOperations.push(delayedOp as DelayedOperation<unknown>); |
There was a problem hiding this comment.
Without this you get:
Type 'DelayedOperation' is not assignable to type 'CancelablePromise'.
for the return delayedOp below.
| @@ -42,7 +42,7 @@ describeFn('WebChannel', () => { | |||
| const makeUrl = conn.makeUrl.bind(conn); | |||
|
|
|||
| it('includes project ID and database ID', () => { | |||
| const url = makeUrl('Commit', {}); | |||
| const url = makeUrl('Commit'); | |||
There was a problem hiding this comment.
This was just a bogus extra parameter.
| ) as ApiDescribe; | ||
| apiDescribe.skip = apiDescribeInternal.bind(null, describe.skip); | ||
| apiDescribe.only = apiDescribeInternal.bind(null, describe.only); | ||
|
|
There was a problem hiding this comment.
Without these changes, we were getting two errors:
Argument of type 'PendingSuiteFunction' is not assignable to parameter of type 'IContextDefinition'. Type 'PendingSuiteFunction' is missing the following properties from type 'IContextDefinition': only, skip
For passing describe.skip to apiDescribeInternal() and:
Property 'skip' does not exist on type '(message: string, testSuite: (persistence: boolean) => void) => void'.ts(2339)
for assigning to apiDescribe.skip.
| @@ -106,7 +105,7 @@ export function addEqualityMatcher(): void { | |||
| /*showDiff=*/ true | |||
| ); | |||
| } else if (originalFunction) { | |||
| originalFunction.apply(this, args); | |||
| originalFunction.call(this, expected, msg); | |||
There was a problem hiding this comment.
Without these changes we were getting:
Argument of type 'unknown[]' is not assignable to parameter of type '[unknown, unknown]'.
Type 'unknown[]' is missing the following properties from type '[unknown, unknown]': 0, 1
There was a problem hiding this comment.
Hey! This PR rewrites all my TypeScript voodoo from last year. And now you are asking me to approve a rewrite!? :)
I think there is an issue with the DelayedPromises. It's unrelated to this PR, but it might be a small enough fix to make along with the rest of the changes. If not, we can fix it in a follow up (with a more suitable PR title).
| this, | ||
| timerId, | ||
| delayMs, | ||
| op, | ||
| op => this.removeDelayedOperation(op) | ||
| op => this.removeDelayedOperation(op as DelayedOperation<unknown>) |
There was a problem hiding this comment.
Hmmm. This looks wrong to me. Not the code you wrote - but don't we need to call removeDelayedOperation and delayedOperations.push with delayedOp?
There was a problem hiding this comment.
I agree it looks wrong, but it's actually right because unlike the line above, the removal callback is called with the delayedOp. It's pretty confusing though since we call it "op" which is already an in-scope variable. I renamed it to removedOp to avoid ambiguity. I tried removedDelayedOp but that is a bit long and prettier ended up splitting DelayedOperation<unknown> into 3 lines which looked horrible.
There was a problem hiding this comment.
Oh. Yes, that is confusing. I even checked out the branch locally and it didn't clear up this confusion :/ Thanks for renaming.
…en/firestore-strict-bind-call-apply
mikelehen
changed the base branch from
mikelehen/firestore-strict-noImplicitThis
to
master
No description provided.