Skip to content
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

Remove return Promise.resolve() statements from the codebase #422

Merged
merged 14 commits into from Feb 21, 2018
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions config/tsconfig.base.json
Expand Up @@ -2,6 +2,7 @@
"compileOnSave": false,
"compilerOptions": {
"declaration": true,
"importHelpers": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Because you are doing this in the base tsconfig, any helper fxns, across all packages, that match this flag will be required via tslib. As such, we need to add the tslib dependency to all packages using this base config not just the 3 using async/await

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, done in 12c2b54. I've omitted *-types packages as stated in the commit message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, thanks!

"lib": [
"es5",
"es2015",
Expand Down
16 changes: 6 additions & 10 deletions integration/messaging/test/utils/test-server.js
Expand Up @@ -42,9 +42,9 @@ class MessagingTestServer {
return `http://localhost:${PORT_NUMBER}`;
}

start() {
async start() {
if (this._server) {
return Promise.resolve();
return;
}

return new Promise((resolve, reject) => {
Expand All @@ -56,15 +56,11 @@ class MessagingTestServer {

// Sometimes the server doesn't trigger the callback due to
// currently open sockets. So call `closethis._server
stop() {
if (!this._server) {
return Promise.resolve();
async stop() {
if (this._server) {
this._server.close();
this._server = null;
}

this._server.close();
this._server = null;

return Promise.resolve();
}
}
module.exports = new MessagingTestServer();
3 changes: 2 additions & 1 deletion packages/app/package.json
Expand Up @@ -16,7 +16,8 @@
"license": "Apache-2.0",
"dependencies": {
"@firebase/app-types": "0.1.1",
"@firebase/util": "0.1.9"
"@firebase/util": "0.1.9",
"tslib": "^1.9.0"
},
"devDependencies": {
"@types/chai": "^4.0.4",
Expand Down
3 changes: 2 additions & 1 deletion packages/database/package.json
Expand Up @@ -21,7 +21,8 @@
"dependencies": {
"@firebase/database-types": "0.1.1",
"@firebase/util": "0.1.9",
"faye-websocket": "0.11.1"
"faye-websocket": "0.11.1",
"tslib": "^1.9.0"
},
"devDependencies": {
"@types/chai": "^4.0.4",
Expand Down
3 changes: 1 addition & 2 deletions packages/database/src/api/Database.ts
Expand Up @@ -132,14 +132,13 @@ export class DatabaseInternals {
constructor(public database: Database) {}

/** @return {Promise<void>} */
delete(): Promise<void> {
async delete(): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this be supported in browsers that don't support async/await. Last time I asked - this wasn't being polyfilled.

If it is - that may be a big polyfill.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think TypeScript supports async/await and has downlevel helpers to make the generated code work in ES3/ES5 (https://blog.mariusschulz.com/2016/12/09/typescript-2-1-async-await-for-es3-es5) but we need to see what the resulting code size change is...

(this.database as any).checkDeleted_('delete');
RepoManager.getInstance().deleteRepo((this.database as any).repo_ as Repo);

(this.database as any).repo_ = null;
(this.database as any).root_ = null;
this.database.INTERNAL = null;
this.database = null;
return Promise.resolve();
}
}
3 changes: 2 additions & 1 deletion packages/firestore/package.json
Expand Up @@ -24,7 +24,8 @@
"dependencies": {
"@firebase/firestore-types": "0.2.1",
"@firebase/webchannel-wrapper": "0.2.6",
"grpc": "^1.7.1"
"grpc": "^1.7.1",
"tslib": "^1.9.0"
},
"peerDependencies": {
"@firebase/app": "^0.1.0",
Expand Down
8 changes: 2 additions & 6 deletions packages/firestore/src/api/database.ts
Expand Up @@ -383,11 +383,9 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
}

INTERNAL = {
delete: (): Promise<void> => {
delete: async (): Promise<void> => {
if (this._firestoreClient) {
return this._firestoreClient.shutdown();
} else {
return Promise.resolve();
}
}
};
Expand Down Expand Up @@ -685,13 +683,11 @@ export class WriteBatch implements firestore.WriteBatch {
return this;
}

commit(): Promise<void> {
async commit(): Promise<void> {
this.verifyNotCommitted();
this._committed = true;
if (this._mutations.length > 0) {
return this._firestore.ensureClientConfigured().write(this._mutations);
} else {
return Promise.resolve();
}
}

Expand Down
4 changes: 1 addition & 3 deletions packages/firestore/src/core/event_manager.ts
Expand Up @@ -87,7 +87,7 @@ export class EventManager {
}
}

unlisten(listener: QueryListener): Promise<void> {
async unlisten(listener: QueryListener): Promise<void> {
const query = listener.query;
let lastListen = false;

Expand All @@ -103,8 +103,6 @@ export class EventManager {
if (lastListen) {
this.queries.delete(query);
return this.syncEngine.unlisten(query);
} else {
return Promise.resolve();
}
}

Expand Down
8 changes: 2 additions & 6 deletions packages/firestore/src/core/firestore_client.ts
Expand Up @@ -371,11 +371,7 @@ export class FirestoreClient {
): Promise<T> {
// We have to wait for the async queue to be sure syncEngine is initialized.
return this.asyncQueue
.enqueue(() => {
return Promise.resolve();
})
.then(() => {
return this.syncEngine.runTransaction(updateFunction);
});
.enqueue(async () => {})
.then(() => this.syncEngine.runTransaction(updateFunction));
}
}
8 changes: 3 additions & 5 deletions packages/firestore/src/local/memory_persistence.ts
Expand Up @@ -47,18 +47,16 @@ export class MemoryPersistence implements Persistence {

private started = false;

start(): Promise<void> {
async start(): Promise<void> {
// No durable state to read on startup.
assert(!this.started, 'MemoryPersistence double-started!');
this.started = true;
// No durable state to read on startup.
return Promise.resolve();
}

shutdown(): Promise<void> {
async shutdown(): Promise<void> {
// No durable state to ensure is closed on shutdown.
assert(this.started, 'MemoryPersistence shutdown without start!');
this.started = false;
return Promise.resolve();
}

getMutationQueue(user: User): MutationQueue {
Expand Down
20 changes: 6 additions & 14 deletions packages/firestore/src/remote/persistent_stream.ts
Expand Up @@ -288,13 +288,12 @@ export abstract class PersistentStream<
}

/** Called by the idle timer when the stream should close due to inactivity. */
private handleIdleCloseTimer(): Promise<void> {
private async handleIdleCloseTimer(): Promise<void> {
if (this.isOpen()) {
// When timing out an idle stream there's no reason to force the stream into backoff when
// it restarts so set the stream state to Initial instead of Error.
return this.close(PersistentStreamState.Initial);
}
return Promise.resolve();
}

/** Marks the stream as active again. */
Expand All @@ -319,7 +318,7 @@ export abstract class PersistentStream<
* @param finalState the intended state of the stream after closing.
* @param error the error the connection was closed with.
*/
private close(
private async close(
finalState: PersistentStreamState,
error?: FirestoreError
): Promise<void> {
Expand Down Expand Up @@ -361,8 +360,6 @@ export abstract class PersistentStream<
// could trigger undesirable recovery logic, etc.).
if (finalState !== PersistentStreamState.Stopped) {
return listener.onClose(error);
} else {
return Promise.resolve();
}
}

Expand Down Expand Up @@ -403,16 +400,14 @@ export abstract class PersistentStream<
this.startStream(token);
},
(error: Error) => {
this.queue.enqueue(() => {
this.queue.enqueue(async () => {
if (this.state !== PersistentStreamState.Stopped) {
// Stream can be stopped while waiting for authorization.
const rpcError = new FirestoreError(
Code.UNKNOWN,
'Fetching auth token failed: ' + error.message
);
return this.handleStreamClose(rpcError);
} else {
return Promise.resolve();
}
});
}
Expand All @@ -436,12 +431,10 @@ export abstract class PersistentStream<
stream: Stream<SendType, ReceiveType>,
fn: () => Promise<void>
) => {
this.queue.enqueue(() => {
this.queue.enqueue(async () => {
// Only raise events if the stream instance has not changed
if (this.stream === stream) {
return fn();
} else {
return Promise.resolve();
}
});
};
Expand Down Expand Up @@ -480,16 +473,15 @@ export abstract class PersistentStream<
);
this.state = PersistentStreamState.Backoff;

this.backoff.backoffAndRun(() => {
this.backoff.backoffAndRun(async () => {
if (this.state === PersistentStreamState.Stopped) {
// Stream can be stopped while waiting for backoff to complete.
return Promise.resolve();
return;
}

this.state = PersistentStreamState.Initial;
this.start(listener);
assert(this.isStarted(), 'PersistentStream should have started');
return Promise.resolve();
});
}

Expand Down