Allow document ID to be specified when added to a collection

[jbaldassari]

I'd like to be able to specify a custom document ID when using Collection.add(...) rather than getting an auto-generated ID. this change should be backwards-compatible. If no document ID is specified we should get the existing behavior of an auto-generated ID.

Let me know if you think it would be better to add a new interface for this, like IDocumentCreationOptions or something along those lines. I don't know if it might be useful to be able to pass other optional fields in the future when adding a new document to a collection.

[IjzerenHein]

Hi, I applaud the effort, but did you even test this, cause I'm 100% sure it doesn't work.

[jbaldassari]

Sorry about that. I thought it would be an easy change and just wanted to start the conversation. Was hoping to find time to test it this weekend. I'm pretty new to working with npm modules so I need to figure out how to build these local changes and then import them into my other project to test them. I noticed there were some unit tests in the project, but most of the tests I saw were for negative cases and failures. I'll take another look to see if there are some tests I could modify.

[IjzerenHein]

Alright cool, well we always welcome new members to the whole JavaScript and NPM ecosystem :)

It's possible that the tests failed because Firestore was responding a bit slowly. I see this from time to time. I just increased the timeout for tests to 10sec, so it should be a bit more forgiving. Please let me know what issue you are getting when running the tests, maybe I can help.

Cheers, Hein

[IjzerenHein]

Oh and please pull the latest code to get that timeout change

[jbaldassari]

Thanks @IjzerenHein . The tests are passing for me locally, but still failing with timeout errors in travis.

[2018-11-16T14:53:03.565Z] @firebase/firestore: Firestore (5.5.0): Could not reach Cloud Firestore backend. Backend didn't respond within 10 seconds.

I tried pushing another change to further increase the timeout. As for being able to add new test coverage for my specific change, I tried adding a test to Collection.add.test.js, but it fails at runtime with a strange error. The test case I tried adding locally was:

test(".add can specify custom document ID", async () => {
    const col = new Collection('todos', {
        createDocument: (source, options) => new TodoDoc(source, options)
    });
    const customId = 'abc123';
    const added = await col.add({
        whoop: true
    }, customId).then(doc => doc.id);
    expect(added.id).toBe(customId);
});

And the error I get at runtime:

    TypeError: Class constructor Document cannot be invoked without 'new'

       7 | 
       8 | class TodoDoc extends Document {
    >  9 |  constructor(source, options) {
         |                               ^
      10 |      super(source, {
      11 |          ...(options || {}),
      12 |          schema

      at new TodoDoc (test/Collection.add.test.js:9:31)
      at Collection.createDocument (test/Collection.add.test.js:53:40)
      at Collection.<anonymous> (src/Collection.ts:492:8)
      at src/Collection.ts:7:71
      at Object.<anonymous>.__awaiter (src/Collection.ts:3:12)
      at Collection.add (src/Collection.ts:416:16)
      at Object.add (test/Collection.add.test.js:56:26)

I can't figure out why I'm getting that error. I don't see anything obviously wrong with that class, and it's complaining about some existing code that I didn't touch. However, I'm not sure it was actually being exercised in any of the other test cases.

[IjzerenHein]

I don't see the problem either. Can you drop the modified file in a Gist so I can check it out?

[IjzerenHein]

As for the timeout change. I experimented with this in the past, but when using large timeout it stills borks every now and then. Also, please keep in mind that a Pull request should only contain changes relevant to the thing you are trying to achieve. Changing the timeout has nothing to do with extending the .add() method. Next time please open a new PR for that, so it can be reviewed and tested separately.

[jbaldassari]

Thanks, will do. I'll revert my timeout change for now. I was just testing it out, but it didn't seem to help.

[jbaldassari]

Collection.add.test.js with new test for specifying custom doc IDs: https://gist.github.com/jbaldassari/39dc8735767839044cb3ad6b36775f0e

[IjzerenHein]

James, it seems your mixing up async/await and vanilla Promises. You should either await for the result, or use .then, but you can't do both. I suggest using the await approach as error handling is than automatically handled correctly. Also keep in mind that the json that you are passing in is invalid, so you will get a schema validation error on that.

[jbaldassari]

@IjzerenHein thanks very much for all your help here. I just found out that there is already a way to specify a custom document ID. Rather than using Collection.add I can just create a Document with a reference pointing to the exact document ID that I want to use, and then call set on that reference to populate it. So I guess Collection.add should only be used if you want a randomly-generated ID. I'll close this PR as it does not seem necessary (and actually the Firebase SDK does not seem to expose a way to pass a custom document ID via collection.add either). Thanks for your patience and for pointing me in the right direction.

[IjzerenHein]

@jbaldassari You'r welcome