add webpacker assets to local files for storage if webpacker config set

[cameronjacoby]

Part of stitchfix/erch#1770

Problem:

The asset_sync gem, which we use in FashionThing to add compiled assets to S3, does not support webpacker assets. This is an issue, because we're currently using webpacker in FashionThing to compile React assets, and the React assets are not making it into the S3 bucket, resulting in this 403:

Solution:

Extend the asset_sync gem to support webpacker and point FashionThing to this fork of the gem. Note that this is a temporary solution, since we will no longer be using asset_sync when FashionThing moves to AWS. We can simply remove the gem from FashionThing's Gemfile altogether, and fixops will add all assets (including those compiled by webpacker) to the S3 bucket.

I used AssetSync#341 and AssetSync#345 as inspiration for this solution, pulling in aspects of both based on what makes sense for us.

[cameronjacoby]

@jbusser I'm not sure I understand the implications of removing this block of code. Any insight based on your previous research?

[jbusser]

Hm, now that I look at it, I might want to leave that in -- at least the code on line 28. Otherwise, I'm not entirely sure how or where we enhance the existing assets:precompile task. I wonder if the author of Asset Sync PR 345 expected us to run an explicit rake asset:sync task instead of calling an enhanced rake assets:precompile.

Perhaps we might want the code from https://github.com/AssetSync/asset_sync/pull/341/files#diff-0237b97660767b03106c26b0115c3621R23

??

[jbusser]

@cameronjacoby I'm testing this with my own S3 bucket and AWS access key credentials, and I can confirm that with this branch we have to call an explicit rake assets:sync, which isn't a problem, but something we should be aware of.

And the double downer is that it doesn't actually work as expected. I see an "assets" directory in my S3 bucket, but no "packs" directory.

[jbusser]

Ah, I was wrong, and I'm ok with that!

I had to generate a custom initializer via rails g asset_sync:install --provider=AWS and then edit it to include a line

config.include_webpacker_assets = true

When I ran rake assets:sync, I got my "packs" directory, and the Webpacker asset was inside.

🎉

[cameronjacoby]

Amazing, thanks for thoroughly testing this out!!

Instead of generating the initializer, I'm going to set ASSET_SYNC_INCLUDE_WEBPACKER_ASSETS=true in production, since that aligns with the rest of our asset_sync setup (see heroku config -a fashionthing-production | grep ASSET_SYNC). Sorry, I totally forgot to mention that was needed to get this working!

I'm ok with running rake assets:sync explicitly. Where do I put that? In the circle.yml?

[jbusser]

Oh. Does setting an environment variable work? Don't we need to add something around here https://github.com/stitchfix/asset_sync/blob/master/lib/asset_sync/engine.rb#L15-L43 to make that happen?

As far as where we call rake assets:sync goes, two options come to mind.

First, we can edit circle.yml as you suggest. I think it would make sense to perform the asset sync before we push to Heroku, so perhaps something like

# circle.yml
    commands:
      - git fetch --unshallow
      - bundle exec rake assets:precompile
      - bundle exec rake assets:sync
      - git push git@heroku.com:fashionthing-production.git $CIRCLE_SHA1:refs/heads/master

Option 2 is we let Heroku handle the asset sync, but that means extending the existing assets:precompile task, something closer to what the author of Asset Sync PR 341 was trying to do. Heroku runs assets:precompile automatically when it fails to detect a manifest.json file in the compiled slug. Details here.

If we pursue option 1, we might want to consider committing a dummy manifest.json to git in order to trick Heroku into not running that task. It will no longer be necessary.

[jlambert121]

Just as a heads up on this, when apps move to aws we'll no longer be using this gem anymore.

[jbusser]

@jlambert121 That's correct. This forked gem is a hedge that gives us flexibility around moving Fashionthing to AWS.

[jbusser]

It works, with some caveats.

It does what we need. I'm good with this.

[cameronjacoby]

@jbusser I took this exactly from AssetSync#341, but I also considered this simpler approach:

if Rake::Task.task_defined?("webpacker:compile")
  Rake::Task["webpacker:compile"].enhance do
    if defined?(AssetSync) && AssetSync.config.run_on_precompile
      Rake::Task["assets:sync"].invoke
    end
  end
else
  Rake::Task["assets:precompile"].enhance do
    if defined?(AssetSync) && AssetSync.config.run_on_precompile
      Rake::Task["assets:sync"].invoke
    end
  end
end

It seems strange to me to check AssetSync.config.include_webpacker_assets, since we're already doing that inside of assets:sync. I also feel like it could get us into a situation where assets just don't get synced at all if Rake::Task.task_defined?("webpacker:compile") returns false, but AssetSync.config.include_webpacker_assets returns true (see line 39).

Anyway, I think it may be better to just keep this part simple to focus on which task we're enhancing and make sure assets get synced either way. Let me know what you think!

[jbusser]

Ah, I think I understand what's going on here. I thought your simpler version would work, but I think the implementation we stole that inspired this fork is probably better.

The intention is to ensure that we enhance either the "webpacker:compile" or "assets:precompile" tasks but never both. I have no idea why someone would have the webpacker gem installed and then say "nah, don't sync that like all my other assets," but here we are.

[cameronjacoby]

The intention is to ensure that we enhance either the "webpacker:compile" or "assets:precompile" tasks but never both.

Agreed, and I think the implementation that "inspired" this PR does that with FashionThing's setup (since Rake::Task.task_defined?("webpacker:compile") and AssetSync.config.include_webpacker_assets will both be true), so I'll leave it as is. 👍

[jbusser]

This could be dangerous. Could we use RSpec's stub_const instead? https://relishapp.com/rspec/rspec-mocks/v/2-11/docs/stubbing-constants/stub-defined-constant#stub-nested-constant

[cameronjacoby]

Definitely - I'll update!

[cameronjacoby]

@jbusser Ok, I updated this to include the necessary changes to lib/asset_sync/engine.rb to get ENV['ASSET_SYNC_INCLUDE_WEBPACKER_ASSETS'] to work and enhanced the webpacker:compile rake task so we don't have to explicitly run assets:sync in FashionThing.

I also left one question on how we're enhancing the rake tasks in lib/tasks/asset_sync.rake that I'd love to get your opinion on. 😄