Exit before assets:precompile task if yarn:install fails

[juliantrueflynn]

Summary

I made this change to Webpacker and was recommended by @gauravtiwari to make it to Rails itself:
rails/webpacker#2192

Reason for change: When doing yarn:install if it failed the assets:precompile task would still be invoked. This PR ensures that on failure with yarn:install that assets:precompile will not be invoked.

Other Information

Link to original issue for person who spotted this: rails/webpacker#2185

[juliantrueflynn]

I'm unsure why CI failed on this one with the change I did. Any ideas? They seem to be unrelated to what this PR is about (i.e. actioncable for example).

[sarcas]

I was interested in this, and so I had a dig around.

It seems like the test failures stem from the dummy package.json that is set up for railties testing. When testing asset concatenation, we run the asset:precompile rake task. While the test is running, we set up a temporary rails app (see the test/isolation folders), including this package.json file.

Our yarn installation step can't find the packages it is looking for here, and so fails. Previously, it didn't fail at this step and so the test passed. Happily, this means that the test failures are related to your change. Unhappily, I don't know enough about yarn or the Rails testing setup to know if this is something we can fix by adjusting the test package.json, or if we need to change something else.

Incidentally, it feels like we might want to have a deeper look at the test cases here, in case we're missing other examples that are passing by coincidence. But that feels like another PR.

[juliantrueflynn]

@sarcas Thank you, that was helpful. I will take a look at the specs. I will take a crack at updating the specs in this PR this week and see how big of a task this is. Maybe the webpacker gem tests can give me an idea of how to solve this.

[sarcas]

@juliantrueflynn you're welcome! I had more time this evening, and it looks like the file in question was introduced here: #35184. Interestingly that PR was about yarn install running in these tests. It might be worth talking to the author there and see if they can give some pointers. Good luck!

[y-yagi]

Suggested change

	exit(1) unless $?.success?
	system({ "NODE_ENV" => node_env }, "#{Rails.root}/bin/yarn install --no-progress --frozen-lockfile") || raise

I think raise is more appropriate than exit. This allows to switch to the using exception: true option when only support Ruby >= 2.6.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[jaredbeck]

Fixed by #41824