Skip to content

Comments

Code review feedback#40

Open
omriAckley wants to merge 1 commit intomasterfrom
code-review
Open

Code review feedback#40
omriAckley wants to merge 1 commit intomasterfrom
code-review

Conversation

@omriAckley
Copy link
Contributor

No need to merge this PR. Instead check out the notes below, the files changed tab, make some issues addressing whatever you consider important, then close this PR.

In group.js...

  • you may not need the .then or .catch in your functions if you are just updating the db. Firebase/Firestore will reflect that

  • set up an initial listener for realtime updates, see: https://firebase.google.com/docs/firestore/query-data/listen

  • Also make sure that you remove the listener when you unmount (similar to redux)

  • your collections/documents may be tied to the state, props, or the URL bar in order to obtain the snapshot.

  • Really nice documentation :D

  • db.collection('cities').doc('aergadgaer').update({name: 'LA'});
    {
    name: 'San Francisco',
    color: 'red',
    age: 85
    }

  • db.collection('cities').doc('aergaerg').set({name: 'LA'}, {merge: true});

  • generalize methods to be more reusable, as opposed to specific methods. For very specific methods, you might want to make direct calls.

  • React Toaster => display errors, better than console.log

  • You can move some funcs into a utils file/folder

  • Lack of return statements. All functions should have a return unless you know that it doesn't matter.

  • Remove files you don't need -> test file in firebase, and the server folder.

  • Bluebird.try (for Snippet.js)

Docker!

  • Promisify in /master/docker.js: lines 22 - 24 => they may not be promises & you can only await promises.
    • Have a finally in your async block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant