Need feedback for changes to version 1.0.0

[samuelgozi]

Renaming some Reference methods

Right now a Reference has the next CRUD methods:

• get
• set
• update
• delete

Some are accessible only on references to documents(set, update, delete), and the rest are accessible through references to collections too but have different behavior(set and get).
This can lead to some hard to detect bugs, for example, when we expect to be working on a doc reference, but for some reason(maybe human mistake when writing the path) we have a col ref, we might want to call get() to get the document, but receive an array of documents because get() on references to collections return all of its documents.
Another example can be when calling set() on a collection, a new document will be added with a random ID. If we are expecting to be modifying a specific doc instead of adding one to the collection, it might be hard to debug because no error will be thrown.

My suggested solution is to change some of the method names to make it clearer of what your intentions are, and if you use the wrong method, an error will be thrown making it clearer what the issue was.

I propose the next name changes:

• createDocument - Will be available only on collections and will create a new document with a random id. The old name was set.
• list - Will be available only on collections and will return an array of all of its documents. The old name was get.

Don't return the result of the operations

Right now when calling the methods set and update, the new document resulting from the operation will be returned. This is very helpful when creating new documents with auto-generated IDs, when we want to know it to cache the doc locally. And also, some devs like to know what the final doc looks like in the database.

However, providing this functionality without additional requests is not always guaranteed because of the way Firebase Firestore works. I'll explain; the Firestore API only returns the resulting document for regular write operations. However many times behind the scenes I'm forced to use Transactions because regular writes don't support Transforms which are values that are calculated in the server.

So when a dev passes a transform I'm forced to use a Transaction instead, which makes things more complex. So to keep things simpler(and the lib small) I'm considering always using Transactions behind the scenes, but then I won't be able to return the resulting document when making a set/update operation, and the dev will need to do that by himself.

So if we decide to do this, it will reduce the libs size and improve its performance, and make all write operations work with one request. On the other hand, if we decide to return the resulted document, then on operations including a Transformthe lib will make two requests, one to perform the write operation, and one to read the result and return it. In addition, we need to keep some code for converting the operations to transactions when needed.

Please don't hesitate to write your opinion. And no matter which way we turn, the lib will still be much faster and smaller than the official one, so that is not a concern.

[isokosan]

I like the idea with method name changes. Isn't createDocument already named add in the original SDK? I think using that same name would make most sense.

On transforms, I would vote for always returning the result at the moment for both sets and updates. Reads are cheap in firebase anyways.

[samuelgozi]

I like the idea with method name changes. Isn't createDocument already named add in the original SDK? I think using that same name would make most sense.

On transforms, I would vote for always returning the result at the moment for both sets and updates. Reads are cheap in firebase anyways.

@isokosan Regarding the first one, I think you are right, add makes more sense. I opted for createDocument because that's how its called behind the scenes, but add definitely looks more elegant.

As for the second one, it's not only the cost that I'm concerned about, it's mostly the latency. It will take at least twice as long because its an additional request. And when using this lib on the server-side, that can be an issue. Hoping for more feedback to see how people use it, but I like returning results too.

[sesam]

Hey, nice work! +1 on add. How about returning an object that has the id of the new document, and maybe also some methods that can be called to get the path / Reference / to read back the document from the server. But if you have triggers you anyway need to wait quite some time before reading back is really useful. I think it's a good idea to have realtime & offline as additional layers on top of the base functionality. I was thinking about getting an app to load and render all not editable parts / first load by using this library, maybe through a WebWorker. I was reading up on firebase/firebase-js-sdk#2197 if anyone else here (or from there) is interested.

[samuelgozi]

@sesam Thanks for the feedback!
If we don't decide to return the resulting document, I will still return something when adding a new one. As you said, it will be necessary to have some sort of reference to the newly created doc, at least to know how to how its called.
I think that returning a reference would be a good idea.

In regards to offline and realtime, from the beginning, I was planning to add those features as an extension to the base lib. Something similar to how Auth works with it.

I thought about doing offline support through service workers, but it depends on the browser support we need. And realtime can happen on a web worker, but the REST API still doesn't include the listen(realtime) method...

I opened a few feature requests, but they don't seem to budge.

[samuelgozi]

I just published a beta 1.0.0-beta.8 with major breaking changes:

1. db.reference method renamed to db.ref
2. add,set and update no longer return the updated document after the operation.
3. add returns a reference to the newly created document, but only after the operation succeeded.
4. Refactored Transactions with some minor changes to props in CRUD methods.
5. Refactor of almost all of the code, but it doesn't affect much the API.

I stil haven't tested how the changes affected performance or bundle size, but I expect it to be significantly positive in both metrics.

[samuelgozi]

RC1 for version v1.0.0 was released yesterday.
The documentation was updated with the changes, and a full reference is now automatically generated(link in the README).

From now on, I will stick only to minor changes to the overall API, but if there are any suggestions this is the time.
I'm closing this issue, please open a new one for each suggestion/bug.