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

Support binary files #3144

Merged
merged 4 commits into from Apr 23, 2021
Merged

Conversation

mgilbir
Copy link
Contributor

@mgilbir mgilbir commented Apr 18, 2021

This PR provides a broader fix for the issue #2606 that was previously addressed with PR #2615.

The problem with the previous PR is that it assumes that all files that will be served by a component can be UTF-8 encoded. This is fine as long as it is JS and CSS assets, but breaks down the moment the component needs to serve binary files such as images or font files.

Instead of forcing the reading to apply an UTF-8 encoding, the reader can open the file in binary more and let the writer send bytes to the client. This comes closer to what the handler is trying to do by reading an arbitrary file from disk and sending it to the client.

@jroes
Copy link
Contributor

jroes commented Apr 19, 2021

Hm, yes I do think this might be a better solution. @tconkling wdyt?

Copy link
Collaborator

@kmcgrady kmcgrady left a comment

Choose a reason for hiding this comment

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

Hey @mgilbir This overall looks like a much better solution. I have one comment on a test issue. I also see an issue with merge conflict, so I would just merge in the latest develop branch.

Thought I'd let @danthe3rd in on this to offer the opportunity to double check we don't undo the previous fix. I think reading and sending as binary just is much easier to reason about than worry about encodings.

lib/tests/streamlit/components_test.py Outdated Show resolved Hide resolved
@danthe3rd
Copy link

Hey @mgilbir This overall looks like a much better solution. I have one comment on a test issue. I also see an issue with merge conflict, so I would just merge in the latest develop branch.

Agree with that - looks good to me as well. Also this is how tornado's StaticFileHandler work (which maybe should just be subclassed here, but that's for another time).

@mgilbir
Copy link
Contributor Author

mgilbir commented Apr 23, 2021

Thanks for checking the PR. I've merged in develop and fixed the typo in the test as per @kmcgrady comment.

Copy link
Collaborator

@kmcgrady kmcgrady left a comment

Choose a reason for hiding this comment

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

Awesome. Yes this looks good to me.

Copy link

@invrainbow invrainbow left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Oops, used wrong account. Looks good to me.

@kmcgrady kmcgrady merged commit 76445de into streamlit:develop Apr 23, 2021
@mgilbir mgilbir deleted the fix-component-static-serving branch April 24, 2021 14:23
tconkling added a commit that referenced this pull request Apr 26, 2021
* develop:
  Support binary files (#3144)
  Secrets Management docs (#3065)
  Consolidated startup messages into bootstrap.py and made them pretty. (#3162)
  Merge Small Text feature branch (#3168)
  Stick our script-completion logic into a documented method (#3166)
  Remove path from 404 response (#3165)
  Merge in Git Deploy Menu Item feature (#3154)
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

5 participants