Remove `return Promise.resolve()` statements from the codebase by merlinnot · Pull Request #422 · firebase/firebase-js-sdk · GitHub

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.

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

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!

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

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

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

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.

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

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 😟

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

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.

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.

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

Ready for a review.

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.

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

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

Approved for Database.

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.

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

Again, can you leave the else block?

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?

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

Done ✅

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

                 "compileOnSave": false,
                 "compilerOptions": {
                   "declaration": true,
+                  "importHelpers": true,

                     const fakeSubscription = {
                       endpoint: EXAMPLE_TOKEN_SAVE.endpoint,
-                      unsubscribe: () => Promise.resolve()
+                      unsubscribe: async () => {}

                       });
-                  } else {
-                    // Transient error, just let the retry logic kick in.
-                    return Promise.resolve();

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

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

Remove `return Promise.resolve()` statements from the codebase by merlinnot · Pull Request #422 · firebase/firebase-js-sdk · GitHub

Uh oh!

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

Remove return Promise.resolve() statements from the codebase#422jshcrowthe merged 14 commits intofirebase:masterfirebase/firebase-js-sdk:masterfrom merlinnot:fix-issue-408merlinnot/firebase-js-sdk:fix-issue-408Copy head branch name to clipboard

Conversation

merlinnot commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jshcrowthe commented Jan 10, 2018

Uh oh!

merlinnot commented Jan 10, 2018

Uh oh!

merlinnot commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmidt-sebastian commented Feb 6, 2018

Uh oh!

schmidt-sebastian commented Feb 6, 2018

Uh oh!

merlinnot commented Feb 9, 2018

Uh oh!

merlinnot commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merlinnot commented Feb 12, 2018

Uh oh!

jshcrowthe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merlinnot commented Feb 14, 2018

Uh oh!

jshcrowthe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gauntface left a comment

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jshcrowthe commented Feb 21, 2018

Uh oh!

merlinnot commented Feb 21, 2018

Uh oh!

jshcrowthe commented Feb 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Remove return Promise.resolve() statements from the codebase#422
jshcrowthe merged 14 commits into
firebase:masterfrom
merlinnot:fix-issue-408

merlinnot commented Jan 10, 2018 •
edited
Loading

merlinnot commented Jan 12, 2018 •
edited
Loading

merlinnot commented Feb 10, 2018 •
edited
Loading