Make newConnection() synchronous #3569
Conversation
💥 No ChangesetLatest commit: 234d122 Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂 If these changes should be published to npm, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
Binary Size ReportAffected SDKs
Test Logs
|
| } | ||
|
|
||
| @@ -24,9 +24,9 @@ import { Connection } from '../../remote/connection'; | |||
| export { newConnectivityMonitor } from '../browser/connection'; | |||
|
|
|||
| /** Initializes the HTTP connection for the REST API. */ | |||
| export function newConnection(databaseInfo: DatabaseInfo): Promise<Connection> { | |||
| export function newConnection(databaseInfo: DatabaseInfo): Connection { | |||
| // node-fetch is meant to be API compatible with `fetch`, but its type don't | |||
| export function newConnection(databaseInfo: DatabaseInfo): Promise<Connection> { | ||
| return Promise.resolve(new FetchConnection(databaseInfo, fetch.bind(null))); | ||
| export function newConnection(databaseInfo: DatabaseInfo): Connection { | ||
| return new FetchConnection(databaseInfo, fetch.bind(null)); |
There was a problem hiding this comment.
Can you explain why we need to bind fetch to null here?
There was a problem hiding this comment.
The first argument to any JavaScript function is the "this context", which is this case is not used. It still has to be explicitly set as we are using "bind". If we just pass "bind" as this then calling this function pointer later would use the first argument as the "this" context... and essentially drop it.
Note that we could also do fetch.bind(this), which would be equivalent to using an array function here.
As an example, if we have a function like this:
class Foo {
bar = 'bar';
function foo() {
console.log(this.bar);
}
function test() {
this.foo(); // Prints 'bar';
(() => this.foo())() // Prints 'bar', since arrow functions retain `this` context.
this.foo().bind({bar:'foo'})(); // Different `this`. Prints 'foo'.
}
| @@ -81,6 +84,6 @@ export async function removeComponents(firestore: Firestore): Promise<void> { | |||
| if (datastorePromise) { | |||
| logDebug(LOG_TAG, 'Removing Datastore'); | |||
| datastoreInstances.delete(firestore); | |||
| return (await datastorePromise).termiate(); | |||
| return (await datastorePromise).terminate(); | |||
There was a problem hiding this comment.
s/dataPromise/datastoreInstance?
You don't need the await anymore since, it's no longer a promise.
| @@ -131,7 +125,7 @@ class DatastoreImpl extends Datastore { | |||
| }); | |||
| } | |||
|
|
|||
| async termiate(): Promise<void> { | |||
| async terminate(): Promise<void> { | |||
There was a problem hiding this comment.
Unrelated question to this PR, but why are we making DatastoreImpl.terminate() async if the method isn't? Why does the base Datastore need to have an abstract terminate() that returns Promise<void>, considering the only implementation of the class is DatastoreImpl?
There was a problem hiding this comment.
I don't have an answer for you, so I changed this as well. In general, returning a Promise even when it is not needed can be seen as "future proofing", but this is only really needed for Public APIs that we don't want to break if there is ever a need to make them async.
newConnection()doesn't need to return a Promise, which means that Datastore can be constructed synchronously.