Oleksandr week 2 Node JS #18
Conversation
There was a problem hiding this comment.
You should add this file/folder to the project's git ignore list.
The idea behind loading sensitive details such as API keys from a dedicated file rather than hardcoding them directly into the application files is that the details then don't need to be checked into the project's repository.
There was a problem hiding this comment.
Note the section 3.2.1. in the project instructions. When running the Jest tests, you import the app module, and at the moment that also means starting the server which will fail if the port is already occupied by you running your server there.
Can you think of a way of breaking out only the code necessary to start the server? You should end up with two files:
app.jswhere you define your application, including all the ednpoints. etc. Export theappmodule from this file.server.jswhere you import theappmodule and start the server on a particular port.
| res.status(200).send({ [data.name]: data.main.temp }); | ||
| }); | ||
|
|
||
| app.listen(3000); |
There was a problem hiding this comment.
Not a must but it is quite common to include a short callback function that will somehow notify the user that the server is running. I.e.
app.listen(PORT, () => {
console.log(`Listening on port ${PORT}`);
});
| } | ||
|
|
||
| const cityName = req.body.cityName; | ||
| const url = `https://api.openweathermap.org/data/2.5/weather?q=${cityName}&APPID=${API_KEY}&units=metric`; |
There was a problem hiding this comment.
nit: I like the idea of saving the URL into a variable. Just FYI with apps that have more endpoints, it's more common to save only the fixed part of the URL (the base URL) into a variable, and then include the dynamic parts of the request as part of the function that handles the request.
In this case, this would look like:
const baseurl = `https://api.openweathermap.org/data/2.5/weather`
const response = await fetch(`${baseurl}?q=${cityName}&APPID=${API_KEY}&units=metric`)
| }); | ||
|
|
||
| it("main page works", () => { | ||
| request |
There was a problem hiding this comment.
For consistency/as a best practice, I'd add await here as well.
| import app from "../server.js"; | ||
| import supertest from "supertest"; | ||
| import { API_KEY } from "../sources/keys.js"; | ||
| import nock from "nock"; |
There was a problem hiding this comment.
Nice that you are using a mocking library!
| }); | ||
| }); | ||
|
|
||
| it("user provides gibberish city name", async () => { |
There was a problem hiding this comment.
You could use nock to mock also the failed API request, i.e.
nock("https://api.openweathermap.org")
.get("/data/2.5/weather")
.query({ q: "abcdefg", APPID: API_KEY, units: "metric" })
.reply(404, { message: "city not found" });
| }); | ||
| }); | ||
|
|
||
| it("user does not provide city name", async () => { |
There was a problem hiding this comment.
nit: I reckon anyone reading this would still get what you're testing here but a more descriptive would be something like 'App returns 400 when the city name is missing'. The idea is that you capture also the expected app behavior, not only the user action.
|
Hi Oleksandr, good job overall, I especially like that you ventured into using the |
|
Thank you Michal, your comments really helped me understand things better. Now I know why I should separate server.js and app.js and how to write tests better. |
No description provided.