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

Add /v and /version for command options #13

Closed
wants to merge 1 commit into from

Conversation

tuidamon
Copy link

@tuidamon tuidamon commented Oct 3, 2020

No description provided.

Copy link
Owner

@sanji11 sanji11 left a comment

Choose a reason for hiding this comment

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

Overall looks good. I like the feature you added for checking 301, 307, 308. The change for -v command option does not follow the current method of managing command options.

Comment on lines +11 to +13
if (opts[0] == "/v" || opts[0] == "/version") {
console.log("Current version: 1.0.0");
} else {
Copy link
Owner

Choose a reason for hiding this comment

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

Yargs is been used for managing command options. We should be using this for all options instead of using process.argv.

@sanji11
Copy link
Owner

sanji11 commented Oct 7, 2020

I just tested your code with the added 301, 307, 308 status code feature. Sadly, it did not work.
You did not add redirect: "manual" in the fetch. Without adding that, the URL redirects to new page which returns status code 200. Therefore, it never goes inside the if clause with 301, 307, 308 check.

Here is the website helped me to find the solution:
https://javascript.info/fetch-api#redirect

You can check my code to see more details:)

@sanji11 sanji11 closed this Oct 7, 2020
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.

2 participants