Skip to content

Fix alt asset path #597

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

Closed
wants to merge 2 commits into from

Conversation

wadewinningham
Copy link

Allow for a non-default asset path for compiled assets.

On initial deployments to new servers, skip files we haven't had the opportunity to compile yet.

On an initial Rails application deployment to a server, the assets looked for are not available in the manifest.yml file. Raising an exception prevents the assets from being compiled at all. This change skips missing files silently.
@wadewinningham wadewinningham deleted the fix-alt-asset-path branch September 1, 2016 17:12
@wadewinningham wadewinningham restored the fix-alt-asset-path branch September 1, 2016 17:31
@rmosolgo
Copy link
Member

rmosolgo commented Sep 1, 2016

That change to use the configured prefix looks great, thanks!

Can you help me understand the change which returns "" when an asset is not found? Why is that desirable?

In my own case, if an asset is not available, I'd rather have an error than a silent failure. But I don't use a YAML manifest -- is it different in this case?

@wadewinningham
Copy link
Author

Sorry, I accidentally submitted this PR here rather than my fork of it.

As for returning "", that's a work in progress. The issue we're running into is during deployment. In our Rails 3.2 app, deploying to a new server with the react-rails gem gets to the asset:precompile step and fails because it's looking for the server_rendering.js file in manifest.yml. The problem is that the particular check is prior to the asset being compiled in the first place.

RIght now, by returning "" (which is likely not the most elegant solution) we can successfully deploy and afterwards react-rails seems to work as intended. I'm working on this though to see if there's a better solution and will submit a proper PR once we get there.

@rmosolgo
Copy link
Member

rmosolgo commented Sep 1, 2016

Oh, I see, good luck, I'll keep an eye out for any updates!

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.

2 participants