Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Week 6 #23

Open
wants to merge 31 commits into
base: Week_6
Choose a base branch
from
Open

Week 6 #23

wants to merge 31 commits into from

Conversation

aarushic
Copy link

No description provided.


// console.log(window.localStorage.getItem("username"))

});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you use useEffect, please note the dependency lists you are using. You call this API multiple times unintentionally because you don't use a dependency list in React.

https://devtrium.com/posts/dependency-arrays

uid: doc.id
});

res.status(200).send("OK")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are currently returning text; shouldn't you return the JSON from doc2? As a result, you are getting a

SyntaxError: Unexpected token 'O', "OK" is not valid JSON on the frontend side when you try to convert this text to a json to console log.

//const todos = await todo.where('email', '==', body.email).get();
const ret = todos.docs.map((obj) => obj.data());

// (todos.docs).forEach(doc => console.log(doc.id))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: when you write code that will be reviewed, I'd remove commented out code unless its really necessary for the review / future code changes.

const express = require("express");
const cors = require("cors");
const app = express()
Copy link
Contributor

@Causality-C Causality-C Dec 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You never use this import, please remove




//get
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove all these comments if not in use

const express = require("express");
const cors = require("cors");
const app = express()
const cors = require('cors')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to not have to call cors in the todo.js and instead call it for index.js which will automatically apply cors to all your routes. Test this though and let me know if it works

email: body.email,
});

const doc2 = await db.collection('todos').doc(doc.id).update({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, you don't have to make two calls to the database to augment the todo (since you're duplicating the data).

Just save doc and in the /GET route, augment the data with the uid then:
todos.docs.map(obj => {const data = {...obj.data(), uid: obj.id}; return data})

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.

3 participants