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

Added Features. #22

Merged
merged 2 commits into from
Dec 22, 2022
Merged

Added Features. #22

merged 2 commits into from
Dec 22, 2022

Conversation

paul-1
Copy link
Contributor

@paul-1 paul-1 commented Oct 30, 2022

Yes, I've seen issue #17 and others. But I'm not the one to do major rewrites from scratch, and I liked how this server was small, and could both server files and cgi-ish content. The files being served are mainly css and js and some small graphics. These also benefit from having the ability to have cache control. The forms and table based content became to large to send as a single string body so I added the ability to send data with chunked transfer encoding.

I looked at ampule, but TBH, I had already figured this out here, and I didn't need the extra dependencies either. This is a collection of a few added capabilities. All of the additions should not break current api implementations, just extends the capabilities. Feel free to accept, make comments or just throw away.

This also solves the issue #21 that I was having.

@tekktrik tekktrik requested a review from dhalbert October 31, 2022 01:11
@paul-1
Copy link
Contributor Author

paul-1 commented Oct 31, 2022

I meant to mark this as draft.....for conversation. Plus I see I have some pylint issues to cleanup if there is some desire to continue.

There still is an issue with the route decorator. On some form submissions, it is failing to find the correct route.
I had a version that was just doing a string truncation when the request arrives that was working.

@dhalbert dhalbert marked this pull request as draft October 31, 2022 10:36
@paul-1
Copy link
Contributor Author

paul-1 commented Oct 31, 2022

Well that should clean up the known issues. Not sure how I missed the pull request standards.....sorry for that.

@paul-1 paul-1 marked this pull request as ready for review October 31, 2022 21:06
@dhalbert
Copy link
Contributor

dhalbert commented Dec 2, 2022

Could some folks test this? Thanks.

@paul-1 Could you fix the merge conflict? Thanks.

@paul-1
Copy link
Contributor Author

paul-1 commented Dec 2, 2022

I was looking that the refactoring that happened in #25 the other day. The timeout option is now in the base, but I'll rebase the rest of the functions.

@anecdata
Copy link
Member

@paul-1 Can you briefly describe the use case for the Cache-Control? As I read it, it's purely informative from the server to the client (and possibly any intervening servers), so, for example, the client may choose to hold off on making another request until max-age but the server may still serve up new content to a client still within the freshness interval?

@paul-1
Copy link
Contributor Author

paul-1 commented Dec 18, 2022

The use case is to allow cache for files like css, js, favicon.ico and other static content that you might want to serve as a part of your dynamic web page. The default cache for a file being served is one week.

Full featured web servers would send a response header for the file, that includes the last modified date. While the os module does allow for finding and setting the timestamp on a file, setting the time felt much too complicated for non connected devices. Using the cache control header we let the client keep track of dates before the cache becomes stale. Dynamically generated data using a route decorator does not get a cache header by default. If a log file is being served, then simply override the default cache setting by using a route for the file without defining a cache..

def base(request):
    return HTTPResponse(root_path='/wwwroot', filename='/somelogfile.txt')

or increase the default cache

def base(request):
    return HTTPResponse(root_path='/wwwroot', filename='/banner.png', cache=31_536_000)

Copy link
Member

@anecdata anecdata left a comment

Choose a reason for hiding this comment

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

Tested Cache-Control header and chunked responses with browser, curl (also curl --raw to see the chunks), and from a CircuitPython 8.0.0-beta device. LGTM... thanks @paul-1 !

@anecdata
Copy link
Member

@dhalbert Any further thoughts on this, or good to merge?

@dhalbert dhalbert merged commit 5de66fa into adafruit:main Dec 22, 2022
@dhalbert
Copy link
Contributor

If others have tested, it's fine to merge. Thanks.

@paul-1 paul-1 deleted the my-main branch December 27, 2022 03:01
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Dec 28, 2022
Updating https://github.com/adafruit/Adafruit_CircuitPython_NeoPxl8 to 0.2.0 from 0.1.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_NeoPxl8#4 from adafruit/enhancements

Updating https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k to 1.12.18 from 1.12.17:
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#81 from BiffoBear/detect_5500_error

Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 1.1.0 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#22 from paul-1/my-main

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
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.

3 participants