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

merlinnot · 2018-01-10T12:49:45Z

I cannot make sure all existing tests in the repository pass after these changes since I'm not able to run them. An error message is clear and it says I'd have to create a test project by visiting https://firebase.corp.google.com/ which I'm not allowed to since I'm not a Google employee.

I'd appreciate if someone with access could run all of the tests for me.

gauntface · 2018-01-10T19:59:59Z

packages/database/src/api/Database.ts

@@ -132,14 +132,13 @@ export class DatabaseInternals {
 constructor(public database: Database) {}

 /** @return {Promise<void>} */
- delete(): Promise<void> {
+ async delete(): Promise<void> {


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.

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...

jshcrowthe · 2018-01-10T22:38:57Z

Hey @merlinnot thanks for the heads up, you can run the tests by creating a test project at https://console.firebase.google.com/

In the mean time, I'll get that URL updated! Thanks!

merlinnot · 2018-01-10T23:20:01Z

@jshcrowthe I had a plan to use Travis for testing when I saw you actually run CI for external PRs, glad to see I can do that much easier :) I'll find some time to finish this tomorrow and I'll try to make some size comparison for the built code.

merlinnot · 2018-01-12T06:50:36Z

Tests are failing with quota exceeded but otherwise should pass.

These changes do indeed increase code size, but this increase is almost constant (helper functions) in gzipped versions and allows us to use async/await all over the place 👍

Filename	size diff (PR - master) [bytes]
firebase.js	+4176
firebase.js.gz	+1325
firebase-database.js	+1400
firebase-database.js.gz	+572
firebase-firestore.js	+9254
firebase-firestore.js.gz	+1487
firebase-messaging.js	+2776
firebase-messaging.js.gz	+651

schmidt-sebastian · 2018-02-06T04:07:34Z

@merlinnot Thanks for this cleanup PR! In general, I would be happy to proceed with this if it didn't impact code size as much. Do you think we can find a way to reduce the impact? Adding 9kb to FIrestore is a little hard to swallow, especially since we want to reduce our size in general.

schmidt-sebastian · 2018-02-06T04:08:53Z

Tests are failing with quota exceeded but otherwise should pass.

We have enabled billing for the test projects recently. I have re-kicked your tests to take advantage of this.

merlinnot · 2018-02-09T07:46:47Z

I’ll try to take a look on reducing the size impact over the weekend.

It reduces the size of TypeScript packages (namely `firebase-database`, `firebase-firestore`, `firebase-messaging` and the combined `firebase`) be reusing TypeScript helper methods (e.g. `__awaiter`) which were included in every single file relying on features not available in ES5 (emitted during transpilation process).

merlinnot · 2018-02-10T14:54:04Z

Ok, here we go:

Filename	`7a611b6` (master)	`c1f73c0` (fix-issue-408)	size diff [bytes] (change to master [%])	reaction
firebase.js	406296	407653	+1357 (+0.33)	😕 ❓
firebase.js.gz	117104	118718	+1614 (+1.38%)	😕 ❓
firebase-database.js	178253	175245	-1400 (-1.69)	👌
firebase-database.js.gz	47188	46881	-307 (-0.65)	👌
firebase-firestore.js	270571	270229	-342 (-0.13)	👌
firebase-firestore.js.gz	71108	71000	-108 (-0.15)	👌
firebase-messaging.js	26846	25840	-1006 (-3.75)	👌
firebase-messaging.js.gz	7053	6958	-95 (-1.35)	👌

Some changes were made automatically by Prettier, I guess they've slipped into master somehow (--no-verify? 😉). I decided to leave those as a part of this PR. You can view the relevant changes here and here.

Tests are passing locally, there's something wrong with Travis 😟

merlinnot · 2018-02-12T17:03:07Z

I'll resolve all of the issues once we'll agree it's ready to be merged.

jshcrowthe

In general the changes look good, I like where this is headed. When you remerge w/ master, you should pick up some of these prettier changes which should help to trim down what the actual delta is here.

jshcrowthe · 2018-02-12T23:56:02Z

config/tsconfig.base.json

@@ -2,6 +2,7 @@
 "compileOnSave": false,
 "compilerOptions": {
 "declaration": true,
+ "importHelpers": true,


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

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

Perfect, thanks!

…til packages. Since we set `importHelpers` compiler option to `true` in the base config, we need to add `tslib` to all packages. `*-types` packages are omitted since they do not contain any executable code.

merlinnot · 2018-02-14T20:26:15Z

Merged with master, all changes are meaningful. Travis has some existential problems 😉

Ready for a review.

jshcrowthe

I've reviewed all of the package.json/yarn.lock and top-level config changes and they all look good.

I've given the other files a once over as well and the changes seem fine, though I'll let the other reviewers take a look at those in more depth.

gauntface · 2018-02-15T18:52:30Z

packages/messaging/test/controller-delete-token.test.ts

@@ -205,7 +197,7 @@ describe('Firebase Messaging > *Controller.deleteToken()', function() {
 it(`should handle error on deleteToken ${ServiceClass.name}`, function() {
 const fakeSubscription = {
 endpoint: EXAMPLE_TOKEN_SAVE.endpoint,
- unsubscribe: () => Promise.resolve()
+ unsubscribe: async () => {}


I'm personally not a fan of this pattern - it's difficult to tell what the stubbed behavior should be doing.

It clearly states "do nothing", doesn't it?

It's more about the expectation of the API (i.e. return a Promise).

gauntface

Would rather the stubs were reverted but not enough to argue about it.

schmidt-sebastian

Approved for Database.

mikelehen

Approved for Firestore with 2 minor requests (Josh if it's easier, you can merge this and then you or I can fix them).

I also think this leaves us in a somewhat messy state with mixing usage of await with manual Promise chaining within methods. I think we should work towards cleaning this up, but not in this PR, and in general it's not super urgent.

mikelehen · 2018-02-21T00:31:16Z

packages/firestore/src/remote/remote_store.ts

- } else {
- // Some other error, don't reset stream token. Our stream logic will
- // just retry with exponential backoff.
- return Promise.resolve();
 }


Can you leave the else block (just containing the comment)? It makes the context for the comment more clear.

mikelehen · 2018-02-21T00:31:57Z

packages/firestore/src/remote/remote_store.ts

@@ -798,10 +785,9 @@ export class RemoteStore {
 // another slot has freed up.
 return this.fillWritePipeline();
 });
- } else {
- // Transient error, just let the retry logic kick in.
- return Promise.resolve();


Again, can you leave the else block?

jshcrowthe · 2018-02-21T00:52:46Z

I've verified tests are passing so this is good to go @merlinnot, can you address the two issues from @mikelehen and then we'll get this merged?

@mikelehen

the context more clear. Addresses @mikelehen review firebase#422 (review)

merlinnot · 2018-02-21T19:56:06Z

Done ✅

jshcrowthe · 2018-02-21T21:34:44Z

Awesome work here @merlinnot thanks so much for the contribution!

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

2a83b21

merlinnot requested review from gauntface, mikelehen, schmidt-sebastian and wilhuff as code owners January 10, 2018 12:49

google-oss-bot added the needs-triage label Jan 10, 2018

merlinnot mentioned this pull request Jan 10, 2018

Remove meaningless return statement #420

Closed

merlinnot added 2 commits January 10, 2018 17:29

Fix compilation errors.

7b99b1b

Merge branch 'master' into fix-issue-408

ca16ef7

gauntface reviewed Jan 10, 2018

View reviewed changes

Merge branch 'master' into fix-issue-408

6cdf964

merlinnot force-pushed the fix-issue-408 branch 3 times, most recently from a86e26b to c0c3c63 Compare January 12, 2018 06:24

Fix more compiler errors.

fecde12

merlinnot force-pushed the fix-issue-408 branch from c0c3c63 to fecde12 Compare January 12, 2018 10:12

merlinnot added 2 commits February 10, 2018 13:13

Merge branch 'master' into fix-issue-408

762244b

merlinnot force-pushed the fix-issue-408 branch from 0b0bba4 to 58400cf Compare February 10, 2018 13:40

merlinnot requested a review from jshcrowthe as a code owner February 10, 2018 13:40

[AUTOMATED]: Prettier Code Styling

9cc53ac

merlinnot force-pushed the fix-issue-408 branch from 58400cf to 547d1dc Compare February 10, 2018 13:44

Regenerate top-level yarn.lock

c1f73c0

merlinnot force-pushed the fix-issue-408 branch from 547d1dc to c1f73c0 Compare February 10, 2018 14:39

jshcrowthe suggested changes Feb 12, 2018

View reviewed changes

merlinnot added 2 commits February 14, 2018 19:58

Add tslib as a dependency to app, polyfill, storage, template and u…

12c2b54

…til packages. Since we set `importHelpers` compiler option to `true` in the base config, we need to add `tslib` to all packages. `*-types` packages are omitted since they do not contain any executable code.

Merge remote-tracking branch 'upstream/master' into fix-issue-408

89cfffd

merlinnot requested a review from sphippen as a code owner February 14, 2018 20:07

Revert unnecessary change in comment formatting.

5a3e581

jshcrowthe approved these changes Feb 14, 2018

View reviewed changes

gauntface reviewed Feb 15, 2018

View reviewed changes

gauntface approved these changes Feb 16, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix-issue-408

71923f7

schmidt-sebastian approved these changes Feb 16, 2018

View reviewed changes

mikelehen approved these changes Feb 21, 2018

View reviewed changes

Revert moving comments out of empty else block to make

c329a92

the context more clear. Addresses @mikelehen review firebase#422 (review)

jshcrowthe merged commit 3ee5bdf into firebase:master Feb 21, 2018

merlinnot deleted the fix-issue-408 branch February 21, 2018 22:07

firebase locked and limited conversation to collaborators Oct 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

merlinnot commented Jan 10, 2018 •

edited

gauntface Jan 10, 2018

mikelehen Jan 10, 2018

jshcrowthe commented Jan 10, 2018

merlinnot commented Jan 10, 2018

merlinnot commented Jan 12, 2018 •

edited

schmidt-sebastian commented Feb 6, 2018

schmidt-sebastian commented Feb 6, 2018

merlinnot commented Feb 9, 2018

merlinnot commented Feb 10, 2018 •

edited

merlinnot commented Feb 12, 2018

jshcrowthe left a comment

jshcrowthe Feb 12, 2018

merlinnot Feb 14, 2018

jshcrowthe Feb 14, 2018

merlinnot commented Feb 14, 2018

jshcrowthe left a comment

gauntface Feb 15, 2018

merlinnot Feb 15, 2018

gauntface Feb 16, 2018

gauntface left a comment

schmidt-sebastian left a comment

mikelehen left a comment

mikelehen Feb 21, 2018

mikelehen Feb 21, 2018

jshcrowthe commented Feb 21, 2018

merlinnot commented Feb 21, 2018

jshcrowthe commented Feb 21, 2018

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

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

Conversation

merlinnot commented Jan 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshcrowthe commented Jan 10, 2018

merlinnot commented Jan 10, 2018

merlinnot commented Jan 12, 2018 • edited

schmidt-sebastian commented Feb 6, 2018

schmidt-sebastian commented Feb 6, 2018

merlinnot commented Feb 9, 2018

merlinnot commented Feb 10, 2018 • edited

merlinnot commented Feb 12, 2018

jshcrowthe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlinnot commented Feb 14, 2018

jshcrowthe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauntface left a comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshcrowthe commented Feb 21, 2018

merlinnot commented Feb 21, 2018

jshcrowthe commented Feb 21, 2018

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

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

merlinnot commented Jan 10, 2018 •

edited

merlinnot commented Jan 12, 2018 •

edited

merlinnot commented Feb 10, 2018 •

edited