willhlaw / node-firestore-backup-restore

Google Firebase Firestore backup and restore tool
91 stars 24 forks source link

sorting the keys before writing documents to file #25

Closed chaoranxie closed 6 years ago

chaoranxie commented 6 years ago

Note I am not a js developer. never use flow before. I tested the code using following commands.

npm run build
npm install -g .
firestore-backup-restore --accountCredentials serviceAccountKey.json --backupPath . --prettyPrint
firestore-backup-restore --accountCredentials serviceAccountKey.json --backupPath .
willhlaw commented 6 years ago

Looks good. Thanks for your time! Only thing I see missing is an update to yarn.lock. You can create it now just by running yarn, but it also would have been created if you used yarn to add the lib: yarn add json-stable-stringify. If this lib had a package-lock.json file, then similar would apply for npm. Now, you're this much closer to becoming a js dev 😄.

chaoranxie commented 6 years ago

I see, I just remove the new line in package.json, and ran

yarn add json-stable-stringify

but it looks like yarn.lock did not change at all. if i try

rm yarn.lock
yarn add json-stable-stringify

A lot will change in yarn.lock, is that what you want me to do?

willhlaw commented 6 years ago

Sorry @chaoranxie. I just got to my laptop and checked yarn.lock. Sure enough, json-stable-stringify is already included as a dependency of something else. That's why yarn.lock was not updating.

However, I also noticed there are other versions such as fast-json-stable-stringify in yarn.lock file, which led me to do a little research. This library, https://github.com/nickyout/fast-stable-stringify, has some good performance benchmarks. Native JSON.stringify is ~4 times faster than the stable stringify family of libraries.

Since there are already reports of large amount of files not working with either Mac file descriptor limits or Firestore socket, I think we should concentrate on having this library highly performant by default. I think it will be best to put your improvement behind a --stable | -S option.

Would you be interested in adding that as a command line parameter and adding a section to the README?

chaoranxie commented 6 years ago

Do you mean adding something like following?

    if (prettyPrint === true) {
        if (stable === true) {
            fileContents = stringify(documentBackup, { space: 2 });
        } else {
            fileContents = JSON.stringify(documentBackup, null, 2);
        }
    } else {
        if (stable === true) {
            fileContents = stringify(documentBackup);
        } else {
            fileContents = JSON.stringify(documentBackup, null, 2);   
        }
    }
willhlaw commented 6 years ago

Yes! But you have a pasto on the last line.

    if (prettyPrint === true) {
        if (stable === true) {
            fileContents = stringify(documentBackup, { space: 2 });
        } else {
            fileContents = JSON.stringify(documentBackup, null, 2);
        }
    } else {
        if (stable === true) {
            fileContents = stringify(documentBackup);
        } else {
            fileContents = JSON.stringify(documentBackup);   
        }
    }

And then add the new --stable | -S option around here and assign to a stable variable here like the others.

And then update the readme around here and here right after the prettyPrint section and before the lengthly plainJSONBackup section.

And then...you're awesome.

chaoranxie commented 6 years ago

let me know what you think

willhlaw commented 6 years ago

Just published to npm as version 1.3.0.