Implement Exporting feature by hansottowirtz · Pull Request #372 · rails/sprockets

hansottowirtz · 2016-08-29T19:01:41Z

I created an exporting feature, which replaces manifest.compile. There is now an api for exporters, like there is an api for processors. Gzip is an example of an exporter. You can register an exporter like this:

env.register_exporter 'application/javascript', Sprockets::GzipExporter

There are no breaking changes. I created this api so I could implement a Brotli exporter, which I will use with ngx_brotli. That will look like this:

class BrotliExporter
  def self.call(env, asset, target, dir, logger, wait)
    if File.exist?("#{target}.br")
      logger.debug "Skipping #{target}.br, already exists"
      return
    else
      logger.info "Writing #{target}.br"
      return Concurrent::Future.execute do
        wait.call
        `bro --quality 9 --input #{target} --output #{target}.br`
      end
    end
  end
end

Rails.application.config.assets.configure do |env|
  env.register_exporter %w(text/css application/javascript), BrotliExporter
end

There is room for improvement:

The wait.call needs review, I'm not an expert on concurrency
The syntax needs review: (env, asset, target, dir, logger, wait) is quite long
Makes no use of dependency resolving, I don't know if that can be used here
I am not sure about checking if a file has a certain mime type, is there already some way of checking if 'text/html' matches 'text/*'? If not, I can certainly implement that, it can also be used for processors then.

I will write a guide when this (or something like this) gets merged.

rails-bot · 2016-08-29T19:01:43Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @arthurnn (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

matthewd · 2016-08-29T19:34:26Z

If we're going to return a Concurrent::Future, we might as well pass an Obligation as the wait parameter too (so, the exporter would call .wait! instead of .call). But that's a pretty big if:

We currently do some early work outside the future / before the original write is completed, but at a glance, I see no special reason we must do things that way. In which case, we could put the whole exporter.call invocation inside the future and after the wait, absolving the exporter of any concurrency awareness.

hansottowirtz · 2016-08-30T00:13:27Z

I managed to implement what you said, and I also added a few more changes. An exporter now inherits from the Exporter class, which provides methods like asset, environment, target, but also wait!. An exporter like GzipExporter should wait for FileExporter, otherwise the file will not be found, so it calls wait! FileExporter before gzipping the file. wait! only waits for the exporting process of that asset. An asset can also not be processed more than once by the same exporter. (e.g. that could be the case for text/html and text/*)

matthewd · 2016-08-30T02:14:09Z

That seems rather unduly complicated for the set of exporters we've currently seen -- especially the introduction of a FileExporter.

(Which of my suggested approaches were you following? You moved the creation of the future up to the main method per ＃2, but kept a wait parameter along the lines of ＃1. 😕)

hansottowirtz · 2016-08-30T08:39:39Z

That's true, but it is pretty flexible. But you're right, it might be quite useless for exporters to depend on eachother. I will see what I can do.

hansottowirtz · 2016-08-30T09:27:58Z

An exporter now only waits for FileExporter, which is not run in a future. I want to keep FileExporter seperate, because you might want to unregister it, or only apply it to certain MIME types, which I will implement later. I also made a write method.

hansottowirtz · 2016-09-02T16:50:32Z

I am now using this code in production for exporting Brotli files.

I created the gem sprockets-exporters as a showcase for this PR.

hansottowirtz · 2016-09-02T20:27:05Z

I managed to get a thread pool working.

matthewd · 2016-09-03T02:11:11Z

89e2c73 seemed approachably small, and wanting only modifications. This now feels disproportionately complex, and I'm not particularly interested in trying to walk that back point by point; I'll defer to Team Sprockets if someone else still sees a path forward.

hansottowirtz · 2016-09-03T05:41:48Z

Ok, I will wait for a comment by others. This whole thing was inspired by this comment by @schneems.

Though I have some questions:

Is it better without the Exporter class? I'd say not because it's not really easy to pass another method to call then. However, processors do this too, with an input hash instead of separate arguments. I kinda dislike that syntax, but should exporters do this too?
On my commiting style: Do I need to make less commits and amend them? Also, are my commit messages ok? I'm pretty new to creating pull requests.

schneems · 2016-09-06T20:38:42Z

On my commiting style: Do I need to make less commits and amend them? Also, are my commit messages ok? I'm pretty new to creating pull requests.

Generally it doesn't matter while you're making the PR. We usually ask a feature PR to be squashed at the end into 1 commit. It helps with readability and if we have to revert a feature we can just revert that one commit. I can help you out there if you have questions/problems.

Is it better without the Exporter class? I'd say not because it's not really easy to pass another method to call then. However, processors do this too, with an input hash instead of separate arguments. I kinda dislike that syntax, but should exporters do this too?

Ordered arguments are really hard to use as an API. You can't easily add or remove arguments in the future. That's one reason why hash based APIs like rack are popular because they're easily extensible. The downside is that they can become a rats-nest and become very difficult to work with, even harder to deprecate.

On the overall direction. I'm not sure. The whole concept of archivers is very sprockets-ish in design, i'm not sure if that's good or bad. It does mean we need more thought towards the API as it will outlive anything else we do today. I'll need to bash around some things another day. Maybe I can dig into this soon.

Can you rebase this? I merged a change to the concurrent futures into master and it's conflicting with this PR.

hansottowirtz · 2016-09-06T22:30:42Z

I agree that the APIs need some thought. As I learned Rails a few years ago, I remember assets management and precompilation being one of the most confusing things, so now I'm happy to help working on it.

As for the ordered arguments, I can certainly replace that with a hash but #new isn't really meant for the API, third party modules should just inherit from Exporter and not make changes to new. That was my thought, at least.

schneems · 2016-09-19T20:23:42Z

+      unless mime_types.is_a? Array
+        if mime_types.is_a? String
+          mime_types = [mime_types]
+        elsif mime_types < Exporter


What does mime_types < Exporter do?

It evaluates to true if mime_types is a class that inherits from Exporter (which is different from is_a? because mime_types is a class, not an instance)
http://stackoverflow.com/questions/4545518/test-whether-a-ruby-class-is-a-subclass-of-another-class

I started with #372 and made some style and behavior changes. The idea remains the same, that all writing to disk will happen via an exporter which gives us a place we can modify behavior from a gem. The interface for an exporter is as follows: An exporter is a class, a new instance will be initialized for each asset. It has access to: - asset - environment - directory - target There is a `setup` method that can be used to create mutable state before the exporter is called. A `skip?` method is run synchronously to determine if the exporter should execute or not. Since this is the only method guaranteed to run synchronously and since our logger is not threadsafe, we can do notifications in this method. The logger is passed into `skip?` to prevent people from accidentally storing a reference and trying to log when things are being called in different threads. Finally a `call` method does the work of writing a file to disk. It takes no arguments. Other changes? The top level lib/sprockets dir is really large so I moved the built in exporters to their own directory. Added documentation. Only allow registering of classes for exporters instead of procs. Added a zopfli exporter by default if you have the gem loaded before sprockets gets required. Switch back to using `compile` for the manifest interface. Use promises to chain asset compiles, this also handles #381. Fix tests to not use `rm -rf` with a system call in a few places

schneems · 2016-10-10T15:44:10Z

Zopfli and an archiver interface (called exporters) are merged into master. Check out the docs and source if you need to write one. Or ping me with questions on this thread. Thanks for your input. The final solution was a result of everyone's inputs and help.

Also I pulled in this commit and built on it. Thanks again for all your help!

rails-bot assigned arthurnn Aug 29, 2016

hansottowirtz force-pushed the exporting branch 6 times, most recently from 7ca65a3 to 5978e30 Compare August 30, 2016 00:11

hansottowirtz force-pushed the exporting branch 8 times, most recently from 4bd8594 to 8223167 Compare August 30, 2016 13:56

hansottowirtz force-pushed the exporting branch from d0cbf16 to 41317ac Compare September 2, 2016 20:07

hansottowirtz mentioned this pull request Sep 2, 2016

Use Concurrent::FixedThreadPool to control compressors threads count #339

Merged

This was referenced Sep 2, 2016

Zopfli gzip #233

Closed

[close #233] Allow to register new archivers to make some compressions to asset file #241

Closed

Implement Exporting feature

294fd31

hansottowirtz force-pushed the exporting branch from 666e4c3 to 294fd31 Compare September 6, 2016 22:23

schneems reviewed Sep 19, 2016

View reviewed changes

schneems mentioned this pull request Sep 20, 2016

Exporter interface #386

Merged

schneems closed this Oct 10, 2016

zw963 mentioned this pull request Aug 22, 2021

[Feature Request] Add support for br Content-Type. hmdne/roda-sprockets#11

Closed

Uh oh!

Conversation

hansottowirtz commented Aug 29, 2016

Uh oh!

rails-bot commented Aug 29, 2016

Uh oh!

matthewd commented Aug 29, 2016

Uh oh!

hansottowirtz commented Aug 30, 2016

Uh oh!

matthewd commented Aug 30, 2016

Uh oh!

hansottowirtz commented Aug 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hansottowirtz commented Aug 30, 2016

Uh oh!

hansottowirtz commented Sep 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hansottowirtz commented Sep 2, 2016

Uh oh!

matthewd commented Sep 3, 2016

Uh oh!

hansottowirtz commented Sep 3, 2016

Uh oh!

schneems commented Sep 6, 2016

Uh oh!

hansottowirtz commented Sep 6, 2016

Uh oh!

schneems Sep 19, 2016

Choose a reason for hiding this comment

Uh oh!

hansottowirtz Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schneems commented Oct 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hansottowirtz commented Aug 30, 2016 •

edited

Loading

hansottowirtz commented Sep 2, 2016 •

edited

Loading

hansottowirtz Sep 19, 2016 •

edited

Loading