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

Use baked_file_system shard instead of custom implementation #29

Closed
wants to merge 1 commit into from

Conversation

schovi
Copy link

@schovi schovi commented Jul 5, 2016

@mperham Got inspired by your idea (because it is awesome to have assets inside binary) and parallely made standalone shard, where I learn some Crystal https://github.com/schovi/baked_file_system

I would be honored if you can give me feedack or even accept this PR .)


module Sidekiq
# This could be extracted and genericized as a virtual filesystem macro.
# This work is left as an exercise to the reader.
module Filesystem
class Filesystem
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it need to be a class now?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. Works absolutly same way :)

@schovi schovi force-pushed the feature/baked_file_system branch 4 times, most recently from d2c0d06 to 4732757 Compare July 5, 2016 12:06
@schovi
Copy link
Author

schovi commented Jul 5, 2016

I noticed original version of assets had compression and that is reason why tests for assets fail.
Should this be included in baked_file_system?

It can be done as separate functionality

require "baked_file_system"
require "baked_file_system/compress"

module Files
  BakedFileSystem.load("...")
end
file = Files.get("something")
compressed_file = file.compress
compressed_file.read
compressed_file.mime_type
# etc...

edit: nvm. It is compressed inside binary and you decompress it. Good idea. But for saving space, there should be limit for compression. Some small files are bigger when compressed.

@mperham
Copy link
Collaborator

mperham commented Jul 5, 2016

Something is wrong with the Travis CI Crystal setup so the build always fails. I don't know who to contact or how to fix.

  1. I would like gzip compression support. It halves the size of the web binary, from 3MB to 1.5MB.
  2. It would be nice to have Accept-Encoding: gzip support so we could stream the compressed content to the browser without having to uncompress it.

@mperham
Copy link
Collaborator

mperham commented Jul 5, 2016

Sorry, first of all: thank you for this work. It's great to see my off-hand comment picked up and developed by someone else thousands of miles away. OSS is so cool!

@mperham
Copy link
Collaborator

mperham commented Jul 5, 2016

As for implementing compression, it looks like file outputs the word text or document in files that would compress well, data in files that are more binary. You could use that as a heuristic or simply include a whitelist or blacklist of file extensions.

$ file ~/tax1099k_2014_20150216.pdf 
~/tax1099k_2014_20150216.pdf: PDF document, version 1.7
$ file ~/201507.backup.tbz 
~/201507.backup.tbz: bzip2 compressed data, block size = 900k
$ file README.md 
README.md: ASCII English text
$ file web/assets/javascripts/application.js 
web/assets/javascripts/application.js: UTF-8 Unicode c program text, with very long lines
$ file web/assets/stylesheets/application.css 
web/assets/stylesheets/application.css: ASCII c program text, with very long lines
$ file web/assets/images/logo.png 
web/assets/images/logo.png: PNG image data, 78 x 85, 8-bit/color RGBA, non-interlaced
$ file web/assets/images/favicon.ico 
web/assets/images/favicon.ico: MS Windows icon resource - 2 icons, 16x16, 256-colors

@schovi schovi force-pushed the feature/baked_file_system branch from 4732757 to 6b297bf Compare July 5, 2016 22:11
@mperham
Copy link
Collaborator

mperham commented Jul 8, 2016

Fixed by 0d9765e

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