Skip to content
This repository was archived by the owner on Feb 24, 2022. It is now read-only.
/ reddit-viewer Public archive
  • Sponsor
  • Notifications You must be signed in to change notification settings
  • Fork 6

Fix loginUser #43

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix loginUser #43

wants to merge 1 commit into from

Conversation

Mathieubola
Copy link

Fixed loginUser by sending information through a form data. I also change the library used for the request to "Request" because I don't know how to send form data with the previous one.

Again, this is my first contribution to a project so feel free to critique my work and modify it if you want.

What type of contribution is this?

Bug
  1. The user could not login
New feature
  1. Display the error sent back from reddit (bad username or password)

Fixed loginUser by sending information through a form data. I also change the library used for the request to "Request" because I don't know how to send form data with the previous one.
@ekarbe ekarbe self-requested a review February 17, 2021 08:42
@ekarbe
Copy link
Owner

ekarbe commented Feb 17, 2021

I will add some comments to the code, but I don't accept the idea of adding the request library. We don't need two libraries essentialy doing the same. And request - even though still popular - has been deprecated since a year ago.

You can do the same with the create function of the rest-client.

rest.create<any>(url, {
additionalHeaders: {
"user-agent": "Reddit-Viewer / 2.0.0"
var request = require('request');
Copy link
Owner

Choose a reason for hiding this comment

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

Bad practice. Use import at start of file.

additionalHeaders: {
"user-agent": "Reddit-Viewer / 2.0.0"
var request = require('request');
var options = {
Copy link
Owner

Choose a reason for hiding this comment

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

Bad practice. VAR is global scope. Use LET instead.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I should have worn you that I don't have a lot of experience in JavaScript 😅
I'll fix that next time

if (response.statusCode === 200 && response.result && response.result.json) {
resolve(response.result.json.data);
};
request(options, function (error: { message: any; }, response: { statusCode: string | number; result: { json: { data: any; }; }; body: any }) {
Copy link
Owner

Choose a reason for hiding this comment

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

statusCode: string | number why string?
Is any necessary?

Copy link
Author

Choose a reason for hiding this comment

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

I'll be honest, I generated that part using vscode "quick fix" thing.
I'll make it manually next time

@@ -651,7 +651,6 @@ export async function getMine(data: IAPIData): Promise<IGenericResult<IListing<I
url += "limit=" + data.limit + "&";
}
url += "raw_json=1";
if(!data.cookie){ data.cookie = ""; }
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you remove this?
As far as I remember this fixed a bug where the old session was still used.

Copy link
Author

Choose a reason for hiding this comment

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

I'm pretty sure I didn't remove that, and I don't think vscode would delete it for no reason so I might have made a bad manipulation.

throw reject(error.message);
}
if (response.statusCode === 200 && response.body) {
var responseJson = JSON.parse( response.body ).json;
Copy link

Choose a reason for hiding this comment

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

Better use let or const here here

Suggested change
var responseJson = JSON.parse( response.body ).json;
const responseJson = JSON.parse( response.body ).json;

}
if (response.statusCode === 200 && response.body) {
var responseJson = JSON.parse( response.body ).json;
if (responseJson.errors.length === 0) {
Copy link

Choose a reason for hiding this comment

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

This code should check for falsy responseJson before accessing errors property.

Suggested change
if (responseJson.errors.length === 0) {
if (responseJson && responseJson.errors && responseJson.errors.length === 0) {

Or it is better to add a function to check for validity of response like isValidResponseJson

Suggested change
if (responseJson.errors.length === 0) {
if (isValidResponseJson(responseJson)) {

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

Successfully merging this pull request may close these issues.

None yet

3 participants