Remove assets:precompile task enhancement

[jherdman]

Move this responsibility to the host application

Resolves #125

[tomdracz]

Thanks @jherdman Looking good, just few more bits needed:

• Remove webpacker:yarn_install task from tasks listing https://github.com/shakacode/shakapacker/blob/master/lib/tasks/webpacker.rake#L12
• Remove webpacker:yarn_install references in compile.rake task - https://github.com/shakacode/shakapacker/blob/master/lib/tasks/webpacker/compile.rake (specifically no need for webpacker:yarn_install at https://github.com/shakacode/shakapacker/blob/master/lib/tasks/webpacker/compile.rake#L40 and https://github.com/shakacode/shakapacker/blob/master/lib/tasks/webpacker/compile.rake#L10-L18 doesn't need any deps passed)

[jherdman]

Updated as requested.

[tomdracz]

Two more changes and we're good to go @jherdman

[tomdracz]

We should still use enhance_assets_precompile here so we need something like:

  if Rake::Task.task_defined?("assets:precompile")
    enhance_assets_precompile
  else
    Rake::Task.define_task("assets:precompile" => ["webpacker:compile"])
  end

[tomdracz]

Don't need the yarn_install_available? but we should keep the enhance_assets_precompile method, just without the deps so something like:

def enhance_assets_precompile
  Rake::Task["assets:precompile"].enhance do |task|
    prefix = task.name.split(/#|assets:precompile/).first

    Rake::Task["#{prefix}webpacker:compile"].invoke
  end
end

[jherdman]

smh... oops!

[jherdman]

Fixed. Apologies for the bumps along the way on this one.

[jherdman]

I think we can remove this too? https://github.com/shakacode/shakapacker/blob/master/lib/install/bin/yarn

[justin808]

LGTM

[tomdracz]

I think we can remove this too? https://github.com/shakacode/shakapacker/blob/master/lib/install/bin/yarn

This is still used during setup and diagnostic check so good to remain. All looks good now, merging! Thanks @jherdman

[Judahmeek]

@tomdracz I don't understand the point of this PR.

Isn't yarn install is a no-op if node_modules is already up to date?

[tomdracz]

@tomdracz I don't understand the point of this PR.

Isn't yarn install is a no-op if node_modules is already up to date?

Kinda. Still takes few seconds to fetch and check things. Think Rails also implements yarn:install task (or at least it did before v7) so the task actually gets run twice.

Then you've got stuff like Heroku. You can very easily end up in situation where packages are fetched and reinstalled again when they don't need to. Can't find the issue in the heroku buildpack now but the only solution offered is removing the install task enhancement.

All in all, this is removing bit of "magic" behind the scenes. In the same way as rails server or migrate tasks are not running bundle install, the precompile task should not be installing yarn packages. Principle of least suprise!

[jherdman]

Broadly speaking, too, I'd argue that Shakapacker shouldn't care which package manager I use whatsoever.

[tomdracz]

Valid point @jherdman it was on my mind too. There should be nothing stopping users from npm or similar being used, as long as packages are installed, it's a fair game