Better error handling + general cleanup

punctuations commented

2021-11-10 22:56:43 -03:00

(Migrated from github.com)

I stumbled across this and wanted to do some ~~spring~~ winter cleaning :)
Please let me know if there is anything else that could be done as cleanup and or if I have messed something up!

Additions:

added to .gitignore file
added better error handling with status codes on all requests
refused connections that require POST method with status code 405 (Method not allowed)
added dev command to package.json

Deletions:

cleaned up the .gitignore file, removing /typings and /.next, the project does not use both next.js and typescript.
removed unused parameters in api/post and api/messages
removed an odd space in the server.json file

Question:

one question, however, why are the response messages all 0, -1, -2, -3? I understand that it is what RobTop does but there are definitely ways to go about it that provide more feedback than a cryptic integer.
Is this still being actively maintained/added to or is this just sitting in the background and getting updates only when required?

I stumbled across this and wanted to do some ~~spring~~ winter cleaning :) Please let me know if there is anything else that could be done as cleanup and or if I have messed something up! Additions: + added to .gitignore file + added better error handling with status codes on all requests + refused connections that require POST method with status code 405 (Method not allowed) + added dev command to package.json Deletions: - cleaned up the .gitignore file, removing /typings and /.next, the project does not use both next.js and typescript. - removed unused parameters in api/post and api/messages - removed an odd space in the server.json file Question: - one question, however, why are the response messages all 0, -1, -2, -3? I understand that it is what RobTop does but there are definitely ways to go about it that provide more feedback than a cryptic integer. - Is this still being actively maintained/added to or is this just sitting in the background and getting updates only when required?

🚀 1

GDColon commented

2021-11-10 23:54:24 -03:00

(Migrated from github.com)

Oh this is awesome!!! I appreciate the contribution a lot.
The site is still somewhat maintained but a lot of code is really messy since I started this project a long time ago.

My thoughts

I very much admit that the error handling sucks balls. I mostly just wanted a quick way to check if the response is valid, so I resorted to using RobTop's method of returning -1. I started this project with little webdev experience and part of me feels like changing the current system would take too long to fix and ultimately isn't worth it.
Express sends 200 as the default status code, I'd personally avoid cluttering up the code with .status(200) and would rather make other status codes stand out more
I don't think returning 'method not allowed' is worth it, pretty sure those files can only ever run from post requests anyways
-2 is returned for accurate leaderboard because private servers don't use it. Unlike -1, -2 removes the accurate button on the frontend. Boomlings leaderboard I just don't see a reason to have this work on private servers lol

Oh this is awesome!!! I appreciate the contribution a lot. The site is still somewhat maintained but a lot of code is really messy since I started this project a long time ago. My thoughts - I very much admit that the error handling sucks balls. I mostly just wanted a quick way to check if the response is valid, so I resorted to using RobTop's method of returning -1. I started this project with little webdev experience and part of me feels like changing the current system would take too long to fix and ultimately isn't worth it. - Express sends 200 as the default status code, I'd personally avoid cluttering up the code with .status(200) and would rather make other status codes stand out more - I don't think returning 'method not allowed' is worth it, pretty sure those files can only ever run from post requests anyways - -2 is returned for accurate leaderboard because private servers don't use it. Unlike -1, -2 removes the accurate button on the frontend. Boomlings leaderboard I just don't see a reason to have this work on private servers lol

👍 1

punctuations commented

2021-11-10 23:58:55 -03:00

(Migrated from github.com)

That's fair; however, returning 200 on those things are more just there to clear up (considering the response is just usually integers) confusion of what is and isn't a success, and for method not allowed, it is nice just in case, I personally don't use express in a ton of my projects so I don't even know if they are or are not accessible but if someone does happen to make a request that isn't a post request (this goes for /messages too) then it will return an error code that is able to accurately describe the situation, it can be removed if you wanted but I just wanted to clear that up, at the end of the day it is your project!

punctuations commented

2021-11-11 00:00:35 -03:00

(Migrated from github.com)

And for private servers do you want the status code updated from 418 (I'm a teapot) or not? I will make a commit to remove the comments around that and clarify to anyone in the future with a new comment on what it does.

punctuations commented

2021-11-11 00:04:07 -03:00

(Migrated from github.com)

and the last thing, this is totally just a suggestion, if you do get a lot of people checking this out still it may be worthwhile to make an issue and pin it just to convert all the images in /assets to .webp to decrease loading times :)

punctuations commented

2021-11-18 19:17:01 -03:00

(Migrated from github.com)

bump

GDColon commented

2021-11-18 19:17:51 -03:00

(Migrated from github.com)

Will get to this eventually, just busy with other things

👍 1

GDColon commented

2021-12-04 19:37:10 -03:00

(Migrated from github.com)

Alright hi again, so everything here looks good and I'm willing to merge this, though you'll have to revisit a few spots since there might be conflicts from the recent icon update. Thanks again for doing this!

punctuations commented

2021-12-05 02:37:06 -03:00

(Migrated from github.com)

Alright hi again, so everything here looks good and I'm willing to merge this, though you'll have to revisit a few spots since there might be conflicts from the recent icon update. Thanks again for doing this!

Conflicts have been fixed!

> Alright hi again, so everything here looks good and I'm willing to merge this, though you'll have to revisit a few spots since there might be conflicts from the recent icon update. Thanks again for doing this! Conflicts have been fixed!

GDColon commented

2021-12-05 13:14:27 -03:00

(Migrated from github.com)

Please don't merge the two icon code files

punctuations commented

2021-12-06 04:52:29 -03:00

(Migrated from github.com)

do you mean the two in the API Route?

sudocode1 commented

2021-12-06 08:59:18 -03:00

(Migrated from github.com)

the codeql security checks related to rate limiting can be ignored because in production a rate limit is enabled, its just not enabled by default (from what i can see, there is no app.use(RL) etc. by default, but it could be later on in the file after where all the other app.use(...) things are), the cross site scripting ones are likely of concern though

you could either just ignore these security checks or implement ratelimiting by default by adding app.use(RL) or app.use(RL2) to get rid of the security warnings

it is better to add ratelimiting by default, but colon can manually ignore the security warning on merge

the codeql security checks related to rate limiting can be ignored because in production a rate limit is enabled, its just not enabled by default (from what i can see, there is no `app.use(RL)` etc. by default, but it could be later on in the file after where all the other `app.use(...)` things are), the cross site scripting ones are likely of concern though you could either just ignore these security checks or implement ratelimiting by default by adding `app.use(RL)` or `app.use(RL2)` to get rid of the security warnings it is better to add ratelimiting by default, but colon can manually ignore the security warning on merge

GDColon commented

2021-12-06 10:25:51 -03:00

(Migrated from github.com)

this has nothing to do with the failed checks, that's on my end for being a bad programmer
but yeah just don't merge icon.js and icon_old.js, i split them up for a reason

this has nothing to do with the failed checks, that's on my end for being a bad programmer but yeah just don't merge icon.js and icon_old.js, i split them up for a reason

sudocode1 commented

2021-12-06 11:12:22 -03:00

(Migrated from github.com)

this has nothing to do with the failed checks, that's on my end for being a bad programmer

ah ok my bad, didnt realise

> this has nothing to do with the failed checks, that's on my end for being a bad programmer ah ok my bad, didnt realise

punctuations commented

2021-12-07 04:52:30 -03:00

(Migrated from github.com)

is this what you were looking for in regards to icon.js and icon_old.js?

I've just reverted them prior to 84d02b7, all other files from that commit are the same.

as a summary:

icon_old.js has been deleted, as it did not exist prior to commit 84d02b7
icon.js' state is that of the contents prior to commit 84d02b7

is this what you were looking for in regards to icon.js and icon_old.js? I've just reverted them prior to 84d02b7, all other files from that commit are the same. as a summary: - icon_old.js has been deleted, as it did not exist prior to commit 84d02b7 - icon.js' state is that of the contents prior to commit 84d02b7

GDColon commented

2021-12-07 10:28:39 -03:00

(Migrated from github.com)

ok what now i'm just confused.
please look at this recent commit, it's what i was referring to

ok what now i'm just confused. please look at [this recent commit](https://github.com/GDColon/GDBrowser/commit/84d02b7271f64f630a29c6062b396b70e810a106), it's what i was referring to

punctuations commented

2021-12-07 15:17:19 -03:00

(Migrated from github.com)

I am as well, you asked me to not merge the icon and icon_old files

GDColon commented

2021-12-07 15:50:35 -03:00

(Migrated from github.com)

Tell you what just leave both icon and icon_old untouched, everyting else is cool and good

GDColon commented

2021-12-07 16:06:11 -03:00

(Migrated from github.com)

Aite I'm gonna merge now then properly clean things up in the next commit I'm about to push, ty for the help!

👍 1

Better error handling + general cleanup #182