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

Beimnet w2 nodejs #26

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

beimnet11
Copy link

No description provided.

Comment on lines 7 to 15
"accepts": "^1.3.8",
"array-flatten": "^1.1.1",
"body-parser": "^1.20.3",
"bytes": "^3.1.2",
"call-bind": "^1.0.7",
"content-disposition": "^0.5.4",
"content-type": "^1.0.5",
"cookie": "^0.6.0",
"cookie-signature": "^1.0.6",

Choose a reason for hiding this comment

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

Are you sure you committed the right package.json file? It looks like it has a lot of dependencies that aren't needed, and it's missing some that are needed in order to run the tests. Please check on this and update

const response = await fetch(url);
const data = await response.json();

if (data.cod === '404') {

Choose a reason for hiding this comment

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

This isn't blocking for the review, but something you can think about - what would happen if you get a different error code from the weather service? What would happen in your code and are you ok with that?

@dyaboykyl dyaboykyl self-assigned this Oct 4, 2024
@beimnet11 beimnet11 requested a review from dyaboykyl October 13, 2024 08:45
Copy link

@dyaboykyl dyaboykyl left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for addressing the feedback!


import express from 'express';

const API_KEY = '5b880f4ee3e43bab62bd1d03019f09c2';

Choose a reason for hiding this comment

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

Looks like this is being used again (it's also defined in key.js). It's always preferable to keep security keys separate from the actual code as they are sensitive information. In fact, in a production setting, you would not even check the api key into github as that could expose it as well.

I won't block on this since the code is working well and you've move on the the next module, but something to keep in mind!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants