Skip to content

migrate Firestore to eslint #1859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Jun 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion packages/firestore/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,30 @@
"extends": "../../config/.eslintrc.json",
"parserOptions": {
"project": "tsconfig.json"
}
},
"plugins": [
"import"
],
"rules": {
"no-console":[ "error", { "allow": ["warn", "error"] }],
"import/no-default-export": "error",
"@typescript-eslint/no-unused-vars": [
"error",
{
"varsIgnorePattern": "^_",
"args": "none"
}
]
},
"overrides": [
{
"files": [
"**/*.test.ts",
"**/test/**/*.ts"
],
"rules": {
"@typescript-eslint/no-explicit-any": "error"
}
}
]
}
1 change: 0 additions & 1 deletion packages/firestore/index.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import firebase from '@firebase/app';
import { FirebaseNamespace } from '@firebase/app-types';
import * as types from '@firebase/firestore-types';
import { Firestore } from './src/api/database';
import { configureForFirebase } from './src/platform/config';
import './src/platform_node/node_init';

Expand Down
11 changes: 6 additions & 5 deletions packages/firestore/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@
"build": "rollup -c",
"build:console": "node tools/console.build.js",
"dev": "rollup -c -w",
"lint": "tslint -p tsconfig.json -c tslint.json 'src/**/*.ts' 'test/**/*.ts'",
"lint:fix": "tslint --fix -p tsconfig.json -c tslint.json 'src/**/*.ts' 'test/**/*.ts'",
"lint:eslint": "eslint -c .eslintrc.json '**/*.ts' --ignore-path '../../.gitignore'",
"lint": "eslint -c .eslintrc.json '**/*.ts' --ignore-path '../../.gitignore'",
"lint:fix": "eslint --fix -c .eslintrc.json '**/*.ts' --ignore-path '../../.gitignore'",
"prettier": "prettier --write 'src/**/*.js' 'test/**/*.js' 'src/**/*.ts' 'test/**/*.ts'",
"test": "run-s lint test:all",
"test:all": "run-p test:browser test:emulator",
Expand All @@ -33,6 +32,7 @@
"@firebase/logger": "0.1.17",
"@firebase/webchannel-wrapper": "0.2.21",
"@grpc/proto-loader": "^0.5.0",
"@firebase/util": "0.2.20",
"grpc": "1.20.3",
"tslib": "1.9.3"
},
Expand Down Expand Up @@ -82,7 +82,8 @@
"eslint": "5.16.0",
"@typescript-eslint/parser": "1.10.2",
"@typescript-eslint/eslint-plugin": "1.10.2",
"@typescript-eslint/eslint-plugin-tslint": "1.10.2"
"@typescript-eslint/eslint-plugin-tslint": "1.10.2",
"eslint-plugin-import": "2.17.3"
},
"repository": {
"directory": "packages/firestore",
Expand All @@ -99,4 +100,4 @@
],
"reportDir": "./coverage/node"
}
}
}
3 changes: 1 addition & 2 deletions packages/firestore/src/api/blob.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,7 @@ export class Blob {
// instanceof checks.
// For our internal TypeScript code PublicBlob doesn't exist as a type, and so
// we need to use Blob as type and export it too.
// tslint:disable-next-line:variable-name We're treating this as a class name.
export let PublicBlob = makeConstructorPrivate(
export const PublicBlob = makeConstructorPrivate(
Blob,
'Use Blob.fromUint8Array() or Blob.fromBase64String() instead.'
);
2 changes: 0 additions & 2 deletions packages/firestore/src/api/credentials.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,6 @@ export class EmptyCredentialsProvider implements CredentialsProvider {
*/
private changeListener: CredentialChangeListener | null = null;

constructor() {}

getToken(): Promise<Token | null> {
return Promise.resolve<Token | null>(null);
}
Expand Down
32 changes: 10 additions & 22 deletions packages/firestore/src/api/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ import {
validateStringEnum,
valueDescription
} from '../util/input_validation';
// eslint-disable-next-line import/no-duplicates
import * as log from '../util/log';
// eslint-disable-next-line import/no-duplicates
import { LogLevel } from '../util/log';
import { AutoId } from '../util/misc';
import * as objUtils from '../util/obj';
Expand Down Expand Up @@ -103,11 +105,6 @@ import {
UserDataConverter
} from './user_data_converter';

// The objects that are a part of this API are exposed to third-parties as
// compiled javascript so we want to flag our private members with a leading
// underscore to discourage their use.
// tslint:disable:strip-private-property-underscore

// settings() defaults:
const DEFAULT_HOST = 'firestore.googleapis.com';
const DEFAULT_SSL = true;
Expand Down Expand Up @@ -158,7 +155,7 @@ class FirestoreSettings {
readonly forceLongPolling: boolean;

// Can be a google-auth-library or gapi client.
// tslint:disable-next-line:no-any
// eslint-disable-next-line @typescript-eslint/no-explicit-any
credentials?: any;

constructor(settings: PrivateSettings) {
Expand Down Expand Up @@ -298,6 +295,9 @@ class FirestoreConfig {
* The root reference to the database.
*/
export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
// The objects that are a part of this API are exposed to third-parties as
// compiled javascript so we want to flag our private members with a leading
// underscore to discourage their use.
private readonly _config: FirestoreConfig;
readonly _databaseId: DatabaseId;

Expand Down Expand Up @@ -480,7 +480,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {

const databaseInfo = this.makeDatabaseInfo();

const preConverter = (value: unknown) => {
const preConverter = (value: unknown): unknown => {
if (value instanceof DocumentReference) {
const thisDb = this._config.databaseId;
const otherDb = value.firestore._config.databaseId;
Expand Down Expand Up @@ -1116,7 +1116,7 @@ export class DocumentReference implements firestore.DocumentReference {
options: ListenOptions,
observer: PartialObserver<firestore.DocumentSnapshot>
): Unsubscribe {
let errHandler = (err: Error) => {
let errHandler = (err: Error): void => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to turn off the return-type enforcement for very simple functions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule doesn't provide an option to do that currently.
An alternative is

type ErrorHandler = (err: Error) => void;
let errHandler: ErrorHandler = (err) => {
...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote option #1 then (leave as is in the current PR).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TSLint rule had an option to ignore arrow functions, which I was using in Messaging and Installations. If this rule has a similar option, I think it's safe to enable that.

console.error('Uncaught Error in onSnapshot:', err);
};
if (observer.error) {
Expand Down Expand Up @@ -1387,16 +1387,6 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot {

export class QueryDocumentSnapshot extends DocumentSnapshot
implements firestore.QueryDocumentSnapshot {
constructor(
firestore: Firestore,
key: DocumentKey,
document: Document,
fromCache: boolean,
hasPendingWrites: boolean
) {
super(firestore, key, document, fromCache, hasPendingWrites);
}

data(options?: SnapshotOptions): firestore.DocumentData {
const data = super.data(options);
assert(
Expand Down Expand Up @@ -1860,7 +1850,7 @@ export class Query implements firestore.Query {
options: ListenOptions,
observer: PartialObserver<firestore.QuerySnapshot>
): Unsubscribe {
let errHandler = (err: Error) => {
let errHandler = (err: Error): void => {
console.error('Uncaught Error in onSnapshot:', err);
};
if (observer.error) {
Expand All @@ -1882,7 +1872,7 @@ export class Query implements firestore.Query {
asyncObserver,
options
);
return () => {
return (): void => {
asyncObserver.mute();
firestoreClient.unlisten(internalListener);
};
Expand Down Expand Up @@ -2417,7 +2407,6 @@ function resultChangeType(type: ChangeType): firestore.DocumentChangeType {

// We're treating the variables as class names, so disable checking for lower
// case variable names.
// tslint:disable:variable-name
export const PublicFirestore = makeConstructorPrivate(
Firestore,
'Use firebase.firestore() instead.'
Expand All @@ -2444,4 +2433,3 @@ export const PublicCollectionReference = makeConstructorPrivate(
CollectionReference,
'Use firebase.firestore().collection() instead.'
);
// tslint:enable:variable-name
1 change: 0 additions & 1 deletion packages/firestore/src/api/field_path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import {
// The objects that are a part of this API are exposed to third-parties as
// compiled javascript so we want to flag our private members with a leading
// underscore to discourage their use.
// tslint:disable:strip-private-property-underscore

/**
* A FieldPath refers to a field in a document. The path may consist of a single
Expand Down
2 changes: 0 additions & 2 deletions packages/firestore/src/api/field_value.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
* An opaque base class for FieldValue sentinel objects in our public API,
* with public static methods for creating said sentinel objects.
*/
// tslint:disable-next-line:class-as-namespace We use this as a base class.
export abstract class FieldValueImpl implements firestore.FieldValue {
protected constructor(readonly _methodName: string) {}

Expand Down Expand Up @@ -109,7 +108,6 @@ export class NumericIncrementFieldValueImpl extends FieldValueImpl {
// PublicFieldValue can be used interchangeably in instanceof checks.
// For our internal TypeScript code PublicFieldValue doesn't exist as a type,
// and so we need to use FieldValueImpl as type and export it too.
// tslint:disable-next-line:variable-name We treat this as a class name.
export const PublicFieldValue = makeConstructorPrivate(
FieldValueImpl,
'Use FieldValue.<field>() instead.'
Expand Down
14 changes: 8 additions & 6 deletions packages/firestore/src/api/user_data_converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ import * as firestore from '@firebase/firestore-types';
import { Timestamp } from '../api/timestamp';
import { DatabaseId } from '../core/database_info';
import { DocumentKey } from '../model/document_key';
import { FieldValue, NumberValue, ObjectValue } from '../model/field_value';
import {
FieldValue,
NumberValue,
ObjectValue,
ArrayValue,
BlobValue,
BooleanValue,
Expand All @@ -33,6 +35,7 @@ import {
StringValue,
TimestampValue
} from '../model/field_value';

import {
FieldMask,
FieldTransform,
Expand All @@ -47,8 +50,7 @@ import { assert, fail } from '../util/assert';
import { Code, FirestoreError } from '../util/error';
import { isPlainObject, valueDescription } from '../util/input_validation';
import { primitiveComparator } from '../util/misc';
import * as objUtils from '../util/obj';
import { Dict } from '../util/obj';
import { Dict, forEach, isEmpty } from '../util/obj';
import { SortedMap } from '../util/sorted_map';
import * as typeUtils from '../util/types';

Expand Down Expand Up @@ -394,7 +396,7 @@ export class UserDataConverter {

let fieldMaskPaths = new SortedSet<FieldPath>(FieldPath.comparator);
let updateData = ObjectValue.EMPTY;
objUtils.forEach(input as Dict<unknown>, (key, value) => {
forEach(input as Dict<unknown>, (key, value) => {
const path = fieldPathFromDotSeparatedString(methodName, key);

const childContext = context.childContextForFieldPath(path);
Expand Down Expand Up @@ -544,14 +546,14 @@ export class UserDataConverter {
private parseObject(obj: Dict<unknown>, context: ParseContext): FieldValue {
let result = new SortedMap<string, FieldValue>(primitiveComparator);

if (objUtils.isEmpty(obj)) {
if (isEmpty(obj)) {
// If we encounter an empty object, we explicitly add it to the update
// mask to ensure that the server creates a map entry.
if (context.path && context.path.length > 0) {
context.fieldMask.push(context.path);
}
} else {
objUtils.forEach(obj, (key: string, val: unknown) => {
forEach(obj, (key: string, val: unknown) => {
const parsedValue = this.parseData(
val,
context.childContextForField(key)
Expand Down
7 changes: 4 additions & 3 deletions packages/firestore/src/core/event_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ import { ObjectMap } from '../util/obj_map';
import { Query } from './query';
import { SyncEngine, SyncEngineListener } from './sync_engine';
import { OnlineState, TargetId } from './types';
import { DocumentViewChange } from './view_snapshot';
import { ChangeType, ViewSnapshot } from './view_snapshot';
import { DocumentViewChange, ChangeType, ViewSnapshot } from './view_snapshot';

/**
* Holds the listeners and the last received ViewSnapshot for a query being
Expand Down Expand Up @@ -72,7 +71,9 @@ export class EventManager implements SyncEngineListener {

listener.applyOnlineStateChange(this.onlineState);

if (queryInfo.viewSnap) listener.onViewSnapshot(queryInfo.viewSnap);
if (queryInfo.viewSnap) {
listener.onViewSnapshot(queryInfo.viewSnap);
}

if (firstListen) {
return this.syncEngine.listen(query).then(targetId => {
Expand Down
5 changes: 2 additions & 3 deletions packages/firestore/src/core/firestore_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,6 @@ export class FirestoreClient {
if (!this.canFallback(error)) {
throw error;
}

console.warn(
'Error enabling offline persistence. Falling back to' +
' persistence disabled: ' +
Expand Down Expand Up @@ -431,14 +430,14 @@ export class FirestoreClient {

const remoteStoreOnlineStateChangedHandler = (
onlineState: OnlineState
) =>
): void =>
this.syncEngine.applyOnlineStateChange(
onlineState,
OnlineStateSource.RemoteStore
);
const sharedClientStateOnlineStateChangedHandler = (
onlineState: OnlineState
) =>
): void =>
this.syncEngine.applyOnlineStateChange(
onlineState,
OnlineStateSource.SharedClientState
Expand Down
8 changes: 3 additions & 5 deletions packages/firestore/src/core/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,9 @@ export class Query {
let comparedOnKeyField = false;
for (const orderBy of this.orderBy) {
const comp = orderBy.compare(d1, d2);
if (comp !== 0) return comp;
if (comp !== 0) {
return comp;
}
comparedOnKeyField = comparedOnKeyField || orderBy.field.isKeyField();
}
// Assert that we actually compared by key
Expand Down Expand Up @@ -620,10 +622,6 @@ export class FieldFilter extends Filter {

/** Filter that matches on key fields (i.e. '__name__'). */
export class KeyFieldFilter extends FieldFilter {
constructor(field: FieldPath, op: Operator, value: RefValue) {
super(field, op, value);
}

matches(doc: Document): boolean {
const refValue = this.value as RefValue;
const comparison = DocumentKey.comparator(doc.key, refValue.key);
Expand Down
5 changes: 1 addition & 4 deletions packages/firestore/src/core/sync_engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,6 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
.then(remoteKeys => {
const view = new View(query, remoteKeys);
const viewDocChanges = view.computeDocChanges(docs);
// tslint:disable-next-line:max-line-length Prettier formats this exceed 100 characters.
const synthesizedTargetChange = TargetChange.createSynthesizedTargetChangeForCurrentChange(
queryData.targetId,
current && this.onlineState !== OnlineState.Offline
Expand Down Expand Up @@ -388,7 +387,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
): Promise<T> {
assert(retries >= 0, 'Got negative number of retries for transaction.');
const transaction = this.remoteStore.createTransaction();
const wrappedUpdateFunction = () => {
const wrappedUpdateFunction = (): Promise<T> => {
try {
const userPromise = updateFunction(transaction);
if (
Expand Down Expand Up @@ -976,8 +975,6 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
case 'not-current': {
return this.localStore.getNewDocumentChanges().then(
async changes => {
// tslint and prettier disagree about their preferred line length.
// tslint:disable-next-line:max-line-length
const synthesizedRemoteEvent = RemoteEvent.createSynthesizedRemoteEventForCurrentChange(
targetId,
state === 'current'
Expand Down
4 changes: 2 additions & 2 deletions packages/firestore/src/core/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@

import { ParsedSetData, ParsedUpdateData } from '../api/user_data_converter';
import { documentVersionMap } from '../model/collections';
import { Document, NoDocument } from '../model/document';
import { MaybeDocument } from '../model/document';
import { Document, NoDocument, MaybeDocument } from '../model/document';

import { DocumentKey } from '../model/document_key';
import { DeleteMutation, Mutation, Precondition } from '../model/mutation';
import { Datastore } from '../remote/datastore';
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/core/view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ export class View {
}

function compareChangeType(c1: ChangeType, c2: ChangeType): number {
const order = (change: ChangeType) => {
const order = (change: ChangeType): 0 | 1 | 2 => {
switch (change) {
case ChangeType.Added:
return 1;
Expand Down
Loading