mike/bash-web-server
1
0
Fork 0
Code Issues Pull requests 2 Projects Releases Packages Wiki Activity Actions

Add new features #3

Open
Cosmoledo wants to merge 5 commits from Cosmoledo/add-new-features into main
pull from: Cosmoledo/add-new-features
merge into: mike:main
Conversation 6 Commits 5 Files changed 4 +253 -32
Cosmoledo commented 2025-07-20 13:22:34 -04:00 (Migrated from github.com)
Copy link

Hey, I really liked this project and your video was very informative.

So, I decided to experiment with this server and how it serves my website.

I have made some changes and I am very interested what you think about them:

  • Added some more mime-types
  • Some more log statements
  • Option to inline/embed assets into send html to reduce server load
    • So, it takes longer to get the initial HTML, but there should be less server calls
    • Although my accept on bash 5.3.0(1)-release was already kinda fast
    • "Fancy" function to convert images to base64

Maybe that could be a project/video on it's own: How to parse, validate and modify a HTML string within Bash 😄

Hey, I really liked this project and your video was very informative. So, I decided to experiment with this server and how it serves my website. I have made some changes and I am very interested what you think about them: - Added some more mime-types - Some more log statements - Option to inline/embed assets into send html to reduce server load - So, it takes longer to get the initial HTML, but there should be less server calls - Although my `accept` on `bash 5.3.0(1)-release` was already kinda fast - "Fancy" function to convert images to base64 Maybe that could be a project/video on it's own: How to parse, validate and modify a HTML string within Bash 😄
Cosmoledo (Migrated from github.com) reviewed 2025-07-20 13:23:27 -04:00
bash-web-server
@ -50,0 +67,4 @@
webp) echo 'image/webp';;
woff) echo 'font/woff';;
woff2) echo 'font/woff2';;
xml) echo 'application/xml';;
Cosmoledo (Migrated from github.com) commented 2025-07-20 13:23:22 -04:00
Copy link

I am particularly curious about your thoughts here

I am particularly curious about your thoughts here
bahamas10 commented 2025-07-27 01:15:13 -04:00 (Migrated from github.com)
Copy link

hey, thank you for the PR - I appreciate you making it and adding new features.

I'm happy to merge some of these features but would prefer them as separate PRs:

  1. add more mime types - that's simple enough to add on its own
  2. extracting the path resolution to its own function - this makes perfect sense to me

As for the inlining elements - i don't want this server to do any modifications of the users data so i don't want this feature in the server. IMO I imagine this process of inlining elements of base64 and modifying the users file being something that happens outside of this code. I.e.: a pre-processor or build script the user runs before they serve the data.

hey, thank you for the PR - I appreciate you making it and adding new features. I'm happy to merge *some* of these features but would prefer them as separate PRs: 1. add more mime types - that's simple enough to add on its own 2. extracting the path resolution to its own function - this makes perfect sense to me As for the inlining elements - i don't want this server to do any modifications of the users data so i don't want this feature in the server. IMO I imagine this process of inlining elements of base64 and modifying the users file being something that happens *outside* of this code. I.e.: a pre-processor or build script the user runs *before* they serve the data.
bahamas10 (Migrated from github.com) reviewed 2025-07-27 01:20:01 -04:00
bahamas10 (Migrated from github.com) left a comment
Copy link

I don't want this base64 stuff in this code, but I still wanted to give you a review - so here are my thoughts.

I don't want this base64 stuff in this code, but I still wanted to give you a review - so here are my thoughts.
bash-web-server
@ -50,0 +67,4 @@
webp) echo 'image/webp';;
woff) echo 'font/woff';;
woff2) echo 'font/woff2';;
xml) echo 'application/xml';;
bahamas10 (Migrated from github.com) commented 2025-07-27 01:15:44 -04:00
Copy link

ha, we call this CHEATING :p

ha, we call this CHEATING :p
bahamas10 (Migrated from github.com) commented 2025-07-27 01:16:08 -04:00
Copy link

variable should be:

  1. local
  2. not uppercased
  3. not quoted
variable should be: 1. `local` 2. not uppercased 3. not quoted
bahamas10 (Migrated from github.com) commented 2025-07-27 01:16:23 -04:00
Copy link

use the fatal function

use the `fatal` function
bahamas10 (Migrated from github.com) commented 2025-07-27 01:16:33 -04:00
Copy link

should be made local

should be made local
bahamas10 (Migrated from github.com) commented 2025-07-27 01:16:44 -04:00
Copy link

external utilities used

external utilities used
bahamas10 (Migrated from github.com) commented 2025-07-27 01:17:04 -04:00
Copy link

bc is external

`bc` is external
bahamas10 (Migrated from github.com) commented 2025-07-27 01:17:18 -04:00
Copy link

use fatal function or just return 1 here

use fatal function or just return 1 here
bahamas10 (Migrated from github.com) commented 2025-07-27 01:17:54 -04:00
Copy link

quote "${keys[@]}"

quote `"${keys[@]}"`
bahamas10 (Migrated from github.com) commented 2025-07-27 01:18:03 -04:00
Copy link

use consistent indenting

use consistent indenting
bahamas10 (Migrated from github.com) commented 2025-07-27 01:19:04 -04:00
Copy link

because this is wrapped in $( ... ) the calls to exit in that function won't actually stop the running script. aka no errors will be caught here.

because this is wrapped in `$( ... )` the calls to `exit` in that function won't actually stop the running script. aka no errors will be caught here.
Cosmoledo commented 2025-08-02 09:42:39 -04:00 (Migrated from github.com)
Copy link

Hey, thank you for your comments. I agree with them.

I was about to update my PR, but you added this file, which name is invalid on windows. So I cannot pull the main branch, I also see no purpose of said file.

grafik
Hey, thank you for your comments. I agree with them. I was about to update my PR, but you added [this file](https://github.com/bahamas10/bash-web-server/blob/main/data/hello%3Fworld.txt), which name is invalid on windows. So I cannot pull the main branch, I also see no purpose of said file. <img width="482" height="228" alt="grafik" src="https://github.com/user-attachments/assets/3b919620-8d07-4471-8e9e-76b34a6695b3" />
bahamas10 commented 2025-08-03 21:18:14 -04:00 (Migrated from github.com)
Copy link

oh how fun - yeah that was for testing weird characters, deleted in the main branch now.

oh how fun - yeah that was for testing weird characters, deleted in the main branch now.
Cosmoledo commented 2025-08-06 14:33:13 -04:00 (Migrated from github.com)
Copy link

Ok, I fixed your comments and put my inline-methods to separate files.
So, that they don't change the server, but they exist in case we want to implement them in the future.

I hope it's now more in line with your initial thoughts 😄

Should I still split everything into multiple PRs or is it ok now?
It should be fine as long as you don't squash.

Ok, I fixed your comments and put my inline-methods to separate files. So, that they don't change the server, but they exist in case we want to implement them in the future. I hope it's now more in line with your initial thoughts 😄 Should I still split everything into multiple PRs or is it ok now? It should be fine as long as you don't squash.
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin Cosmoledo/add-new-features:Cosmoledo/add-new-features
git switch Cosmoledo/add-new-features

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff Cosmoledo/add-new-features
git switch Cosmoledo/add-new-features
git rebase main
git switch main
git merge --ff-only Cosmoledo/add-new-features
git switch Cosmoledo/add-new-features
git rebase main
git switch main
git merge --no-ff Cosmoledo/add-new-features
git switch main
git merge --squash Cosmoledo/add-new-features
git switch main
git merge --ff-only Cosmoledo/add-new-features
git switch main
git merge Cosmoledo/add-new-features
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something isn't working

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
question

Further information is requested

  
wontfix

This will not be worked on

No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
mike/bash-web-server!3
No description provided.