Wait for migrations with async calls, even if they don't return a Promise

[bdiz]

The following migration script has an error in it. serverTimestamp is missing parens.

module.exports.migrate = ({firestore, FieldValue}) => {
  firestore.collection("users").get()
    .then((querySnapshot) => {
      querySnapshot.forEach(async (queryDocumentSnapshot) => {
        const id = queryDocumentSnapshot.id;
        const data = queryDocumentSnapshot.data();

        await firestore.collection("profiles").doc(id).set({
          createdAt: FieldValue.serverTimestamp,
          userId: id
        }); 
      });
    });
};

Dryrun does not catch this. It may be difficult for fireway to be able to detect such errors.

But what this issue is about is that the document stored by fireway sets success to true even though this error is thrown:

(node:19102) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 63)
(node:19102) UnhandledPromiseRejectionWarning: Error: Cannot encode value: serverTimestamp() {
        return ServerTimestampTransform.SERVER_TIMESTAMP_SENTINEL;
    }

My expectation is that the fireway document would set success to false.

This is low priority as my understanding is that fireway, whether success is true or false, will not process the migration again until I remove this document.

Thanks.

[bdiz]

I'm not certain, but I think the issue is that I am not properly returning a promise that fireway can wait on. Async operations would still be in progress.

[kevlened]

Yeah, that's the issue. Without returning a promise, fireway can't trap thrown errors. However, even if you return a promise on your current code, you'll run into issues, because querySnapshot.forEach isn't async. A better way is to loop through the docs:

module.exports.migrate = async ({firestore, FieldValue}) => {
  const querySnapshot = await firestore.collection("users").get();
  for (let queryDocumentSnapshot of querySnapshot.docs) {
    const id = queryDocumentSnapshot.id;
    const data = queryDocumentSnapshot.data();

    await firestore.collection("profiles").doc(id).set({
      createdAt: FieldValue.serverTimestamp(),
      userId: id
    });
  }
};

As an aside, you usually don't need to remove any fireway documents to run a migration. Here's the happy path:

1) Create a migration file with a version higher than your last migration 1) Dryrun until you're happy with the results 1) Run fireway 1) If the migration is successful and you want to make more changes, go to step 1 1) If the migration is unsuccessful, remove the failed fireway collection doc, fix the migration file, and go to step 2

The success status is important for fireway to determine if it should continue to run migrations.

[kevlened]

Hmm. In theory, I could use process._getActiveHandles() to see the open handles and poll until there are no open handles from your migration directory. I'll reopen this and make it a feature request. For failure, I may be able to use process.on('unhandledRejection', cb) to see if the migration failed.

[kevlened]

fireway@1.0.0 warns about these sorts of issues and exposes a --forceWait that will allow fireway to handle these open async calls automatically. Thanks for reporting.