Add support for pdf and zip binary types #67
Conversation
|
|
src/drive.ts
Outdated
| if ( | ||
| [ | ||
| 'application/zip', | ||
| 'application/pdf', | ||
| 'application/octet-stream' | ||
| ].includes(file.type) |
There was a problem hiding this comment.
Since this is a condition for using "base64", would you mind extending the if condition that's above with your new condition?
There was a problem hiding this comment.
Long term we'd need some better way to do this, maybe there is some place in jupyter-server where they decide depending on the extension or mimetype whether it should be sent to the front-end in base64 or something else.
Not asking you to figure it out in this PR, but if you do that's great!
There was a problem hiding this comment.
Here you go. Pushed on new commit that groups all base64 conditions together.
There was a problem hiding this comment.
base64 is always safe but it incurs some overhead especially for large text files like CSV. Maybe a safer way would be to reverse the check by explicitly specifying the types that should be treated as text, and defaulting to binary hence base64 format. Maybe the type starting by "text/" + "application/json" would cover a lot of cases.
There was a problem hiding this comment.
Thanks! The CI failures are most likely unrelated. The CI may have been broken since some time.
2 participants
Hi, I hit the issue of not being able to open a ZIP file in JupyterLite due to a binary-to-text corruption at some point. I dug into the code base and I think I managed to fix it (at least it works on my machine 😄).
Thanks ahead for your review!