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

Conversation

@merlinnot
Copy link
Contributor

@merlinnot merlinnot commented Jan 10, 2018

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.

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

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

@gauntface gauntface Jan 10, 2018

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.

Loading

Copy link
Contributor

@mikelehen mikelehen Jan 10, 2018

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

Loading

@jshcrowthe
Copy link
Contributor

@jshcrowthe jshcrowthe commented Jan 10, 2018

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!

Loading

@merlinnot
Copy link
Contributor Author

@merlinnot merlinnot commented Jan 10, 2018

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

Loading

@merlinnot merlinnot force-pushed the fix-issue-408 branch 3 times, most recently from a86e26b to c0c3c63 Jan 12, 2018
@merlinnot
Copy link
Contributor Author

@merlinnot merlinnot commented Jan 12, 2018

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

Loading

@schmidt-sebastian
Copy link
Member

@schmidt-sebastian schmidt-sebastian commented Feb 6, 2018

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

Loading

@schmidt-sebastian
Copy link
Member

@schmidt-sebastian schmidt-sebastian commented Feb 6, 2018

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.

Loading

@merlinnot
Copy link
Contributor Author

@merlinnot merlinnot commented Feb 9, 2018

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

Loading

merlinnot added 2 commits Feb 10, 2018
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 merlinnot requested a review from jshcrowthe as a code owner Feb 10, 2018
@merlinnot
Copy link
Contributor Author

@merlinnot merlinnot commented Feb 10, 2018

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 😟

Loading

@merlinnot
Copy link
Contributor Author

@merlinnot merlinnot commented Feb 12, 2018

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

Loading

Copy link
Contributor

@jshcrowthe jshcrowthe left a comment

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.

Loading

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

@jshcrowthe jshcrowthe Feb 12, 2018

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

Loading

Copy link
Contributor Author

@merlinnot merlinnot Feb 14, 2018

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.

Loading

Copy link
Contributor

@jshcrowthe jshcrowthe Feb 14, 2018

Choose a reason for hiding this comment

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

Perfect, thanks!

Loading

merlinnot added 2 commits Feb 14, 2018
…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 merlinnot requested a review from sphippen as a code owner Feb 14, 2018
@merlinnot
Copy link
Contributor Author

@merlinnot merlinnot commented Feb 14, 2018

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

Ready for a review.

Loading

Copy link
Contributor

@jshcrowthe jshcrowthe left a comment

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.

Loading

@@ -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 () => {}
Copy link
Contributor

@gauntface gauntface Feb 15, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor Author

@merlinnot merlinnot Feb 15, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor

@gauntface gauntface Feb 16, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor

@gauntface gauntface left a comment

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

Loading

Copy link
Member

@schmidt-sebastian schmidt-sebastian left a comment

Approved for Database.

Loading

Copy link
Contributor

@mikelehen mikelehen left a comment

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.

Loading

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

@mikelehen mikelehen Feb 21, 2018

Choose a reason for hiding this comment

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

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

Loading

@@ -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();
Copy link
Contributor

@mikelehen mikelehen Feb 21, 2018

Choose a reason for hiding this comment

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

Again, can you leave the else block?

Loading

@jshcrowthe
Copy link
Contributor

@jshcrowthe jshcrowthe commented Feb 21, 2018

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?

Loading

@merlinnot
Copy link
Contributor Author

@merlinnot merlinnot commented Feb 21, 2018

Done

Loading

@jshcrowthe
Copy link
Contributor

@jshcrowthe jshcrowthe commented Feb 21, 2018

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

Loading

@jshcrowthe jshcrowthe merged commit 3ee5bdf into firebase:master Feb 21, 2018
1 of 2 checks passed
Loading
@merlinnot merlinnot deleted the fix-issue-408 branch Feb 21, 2018
@firebase firebase locked and limited conversation to collaborators Oct 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants