Adding no-unused-vars to TSLint #493
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt-nounusedvars
branch
from
04339fc to
e752597
Compare
schmidt-sebastian
changed the title
packages/firestore/package.json
Outdated
| "scripts": { | ||
| "dev": "gulp dev", | ||
| "lint": "tslint -c tslint.json src/**/*.ts test/**/*.ts", | ||
| "lint": "tslint -p tsconfig.json -c tslint.json firestore src/**/*.ts test/**/*.ts", |
There was a problem hiding this comment.
What is 'firestore' here?
| private handshakeComplete_ = false; | ||
|
|
||
| constructor( | ||
| private databaseInfo: DatabaseInfo, |
There was a problem hiding this comment.
FWIW, removing databaseInfo everywhere is probably creating a porting difference (web doesn't need it because of the way connection is plumbed through vs. the way grpc works on iOS / Android), but it's probably fine.
| if (originalFunction) { | ||
| use(chai => { | ||
| const wrappedDefault = _super => { | ||
| return _super => { |
There was a problem hiding this comment.
How confident are you that this is right? It doesn't seem right to me... Seems like this should be calling Assertion.overwriteMethod() to undo what beforeEach() did... Maybe ping Josh or log a bug?
| const components: FieldValue[] = []; | ||
| for (const value of values) { | ||
| const [field, dataValue] = value; | ||
| const dataValue = value[1]; |
There was a problem hiding this comment.
This is less readable. :-/ Is there any other option? a placeholder instead of field? What if you bump this into the for loop? (i.e. for (const [field, dataValue] of values) { ... }
| return documentMutationsStore(txn) | ||
| .iterate({ range: startRange, keysOnly: true }, (key, _, control) => { | ||
| const [userID, keyPath, batchID] = key; | ||
| const [userID, keyPath] = key; |
There was a problem hiding this comment.
I don't love this change. Perhaps we should use the ignore-pattern option of no-unused-variable to exclude _blah variables, and then name this _batchId or something?
| ); | ||
| } else { | ||
| _super.apply(this, args); | ||
| } |
There was a problem hiding this comment.
FWIW, I suspect this code works but is a bit bonkers. We'll end up overwriting the method multiple times and I think each _super will point to the previous method, so as this code executes repeatedly you'll end up with a chain of _super's pointing to each other and so _super() will call _super() will call _super() until you finally get back to the original function which won't call _super()... To make things extra bonkers, there will be multiple instances of the isActive variable (one for each time addEqualityMatcher() has been called since the tests started)...
But I don't really care. :-)
There was a problem hiding this comment.
I mostly uploaded this to give Josh a chance to look at this... but now that I have your attention:
- The existing code never actually performed any cleanup and even if it had, this cleanup would only have worked if deepEqual had ever been called.
- With the new cleanup, some tests started failing that compared Map values directly. I added the equality helper to these tests.
- I removed some of the 'bonkersness' by keepin the original function around. This should reduce the chain of _super._super._super calls.
schmidt-sebastian
force-pushed
the
mrschmidt-nounusedvars
branch
from
f133c4b to
d710884
Compare
3 participants
This PR adds type checking to tslint and enables "no-unused-vars".
I also added a new auto-fix option to the default scripts (
yarn lint:fix), which is responsible for some of the changes in here.Tests are all passing locally for me and
yarn prepareworks in the repo root as well as in packages/firestore