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 the basics to add serving of static files #36

Merged
merged 2 commits into from
Jul 5, 2021

Conversation

sansyrox
Copy link
Member

@sansyrox sansyrox commented Jul 2, 2021

First implementation to enable the serving of static files

@JackThomson2
Copy link
Contributor

Just an idea can't we load the file from rust and not touch python at all, I.e add app.serve("localpath", "publication")

@sansyrox
Copy link
Member Author

sansyrox commented Jul 3, 2021

Just an idea can't we load the file from rust and not touch python at all, I.e add app.serve("localpath", "publication")

@JackThomson2 , I can see three problems in this, do correct me if I am wrong:

  1. How will you check the routing ?
  2. It will also not allow us to give html response on HTTP methods apart from GET.
  3. Non standard syntax and how will we add support for multiple HTML files?

@JackThomson2
Copy link
Contributor

JackThomson2 commented Jul 3, 2021

Hi so,

  1. Routing is given in a way you pass in location on disk and location on api e.g. app.serve("./static/index.html", "/") We would also need to hand mounting a whole directory so app.serve("./static/", "/") would load the whole static directory
  2. We could add an overload to the function to accept different requests, however that seems an unusual use-case.
  3. May be unusual from a python developers POV, but the benefits are massive, we'd be able to achieve true parallelism as we would never touch python. And for handling multiple files I mentioned above in 1. We will allow them to call serve multiple times as we just store them in another concurrent map

Also a note if we do plan to use this approach, we should use the built-in async file reading so it wont block

Jack

@sansyrox
Copy link
Member Author

sansyrox commented Jul 3, 2021

I can definitely understand the benefits of the approach mentioned by you. The decorator syntax is really loved in the python ecosystem.

We can implement both of the approaches. The current approach(including python) will be used to serve smaller single html files and it will also be used to serve json responses.

We will also allow the way you suggested to serve larger html files and mounting directories.

This way we will still preserve the elegance of the code when required. Since no one really serves directories as a response to POST requests but single html files are not so uncommon.

We could add an overload to the function to accept different requests,

Can you please share a snippet on how overloading is done in rust? I could not find it anywhere.

we should use the built-in async file reading so it wont block

Are you talking about this? https://docs.rs/async-fs/1.5.0/async_fs/

@JackThomson2
Copy link
Contributor

Interesting, I've never really seen HTML returned from a POST request, I always thought POST as submitting data to the server and GET getting data from the server. For overloading, i tend to have two methods with similar names and the one with less parameters fills in the default when calling the other. I guess this may be more appropriate as we're using tokio: https://docs.rs/tokio/1.8.0/tokio/fs/index.html

Jack

@sansyrox sansyrox merged commit 0febe58 into main Jul 5, 2021
@sansyrox sansyrox deleted the static_file_implementation branch July 5, 2021 09:31
@JackThomson2
Copy link
Contributor

Dont you need at be setting correct MIME types, as a browser won't like static files returned this way

@sansyrox sansyrox restored the static_file_implementation branch July 5, 2021 09:47
@sansyrox
Copy link
Member Author

sansyrox commented Jul 5, 2021

@JackThomson2 , even though it worked well on my browser, I believe that it will be a good practice if we set mime types.

Screenshot 2021-07-05 at 3 20 04 PM

@sansyrox sansyrox deleted the static_file_implementation branch July 5, 2021 09:51
@JackThomson2
Copy link
Contributor

I think errors would start to appear when using JS or other file types.

@sansyrox
Copy link
Member Author

sansyrox commented Jul 5, 2021

I think errors would start to appear when using JS or other file types.

That could be true. I will implement it in a new PR.

@sansyrox
Copy link
Member Author

sansyrox commented Jul 5, 2021

Also, will we have to manage application/json and text/json separately ?

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.

None yet

2 participants