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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor AssetUrlHelper #7927

Merged
merged 16 commits into from Oct 15, 2012

Conversation

Projects
None yet
8 participants
@josh
Member

josh commented Oct 12, 2012

Follow up #7923. Theres a couple things happening here.

asset_path, asset_url

Originally inspired the current sprockets rails plugin, bd38d9f adds general purpose asset_path and asset_url helpers. All the other helpers simply funnel through this main point. Makes it possible for extensions to change the behavior of all the helpers w/o redefining each.

Remove AssetPaths

There doesn't not a whole lot of documentation indicating that this class was ever okay to use publicly. And theres no tests asserting it either. Just 馃敟ing it.

The core of it is replaced by inline helper methods, compute_asset_path and compute_asset_host. Both these methods should now be considered public. Apps or plugins can call them directly or override them to change there behavior.

Isolate old asset id code

I've move all the old asset id mtime query stuff into a separate module, AssetIdHelper. It uses the now compute_asset_path to set itself up.

Since this is now modular, it wouldn't be a bad time to kill it or move it out into an optional plugin. I'd like to get @dhh's take on this.

  • 馃敟 it
  • extract it into a plugin
  • or just leave it as is

Bonus

You should be able to include ActionView::Helpers::AssetUrlHelper into AC::Base just fine now.

/cc @dhh @steveklabnik @guilleiguaran @rafaelfranca

josh added some commits Oct 12, 2012

Refactor AssetUrlHelper to make it friendly for plugins and extensions
Add asset_path/url helper for a consolidated entry point
Expose compute_asset_path as a public API
Expose compute_asset_host as a public API
Move RAILS_ASSET_ID to its own module, AssetIdHelper
Removed AV::AssetPaths
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 12, 2012

Member

About AssetIdHelper I would extract it to a plugin.

Member

rafaelfranca commented Oct 12, 2012

About AssetIdHelper I would extract it to a plugin.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 12, 2012

Member

Awesome work!!!

I'm giving my 馃憤. I just want to wait @dhh to take a look on this one, specially on the AssetIdHelper.

Member

rafaelfranca commented Oct 12, 2012

Awesome work!!!

I'm giving my 馃憤. I just want to wait @dhh to take a look on this one, specially on the AssetIdHelper.

javascript: '/javascripts',
stylesheet: '/stylesheets',
video: '/videos'
}

This comment has been minimized.

@robin850

robin850 Oct 14, 2012

Member

Very nice ! :) 鉂わ笍

@robin850

robin850 Oct 14, 2012

Member

Very nice ! :) 鉂わ笍

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Oct 15, 2012

Member

Cool 馃憤. Looks fine for a plugin imo.

Member

carlosantoniodasilva commented Oct 15, 2012

Cool 馃憤. Looks fine for a plugin imo.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Oct 15, 2012

Member

I'm wondering who would even use the AssetId plugin? Are we talking about people holding out and not going with the asset pipeline? I don't think it's a reasonable assumption to run Rails 4.0 without the asset pipeline, or an alternative, and want Rails to provide AssetIds. I say nuke the fucker. If anyone really does turn out to need it, we can point them to the code that was 馃敟'ed and they can make their own.

Member

dhh commented Oct 15, 2012

I'm wondering who would even use the AssetId plugin? Are we talking about people holding out and not going with the asset pipeline? I don't think it's a reasonable assumption to run Rails 4.0 without the asset pipeline, or an alternative, and want Rails to provide AssetIds. I say nuke the fucker. If anyone really does turn out to need it, we can point them to the code that was 馃敟'ed and they can make their own.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 15, 2012

Member

@dhh yes we are taling about people who not use asset pipeline. If this is the direction that we want to follow so lets 馃挜 it.

Member

rafaelfranca commented Oct 15, 2012

@dhh yes we are taling about people who not use asset pipeline. If this is the direction that we want to follow so lets 馃挜 it.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Oct 15, 2012

Member

Yes, nuke it. Rails 4.0 is the asset pipeline way. If you want to do your own thing, that's fine too. You'll just have to wire it together yourself.

On Oct 15, 2012, at 8:57, Rafael Mendon莽a Fran莽a notifications@github.com wrote:

@dhh yes we are taling about people who not use asset pipeline. If this is the direction that we want to follow so lets it.


Reply to this email directly or view it on GitHub.

Member

dhh commented Oct 15, 2012

Yes, nuke it. Rails 4.0 is the asset pipeline way. If you want to do your own thing, that's fine too. You'll just have to wire it together yourself.

On Oct 15, 2012, at 8:57, Rafael Mendon莽a Fran莽a notifications@github.com wrote:

@dhh yes we are taling about people who not use asset pipeline. If this is the direction that we want to follow so lets it.


Reply to this email directly or view it on GitHub.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Oct 15, 2012

Member

Kinda stuck on the railties tests. They load sprockets-rails which depends on the old AssetPaths api.

Whats the best way to update this stuff w/o making the build red? Maybe create a new branch on sprockets-rails that works w/ this pull?

Member

josh commented Oct 15, 2012

Kinda stuck on the railties tests. They load sprockets-rails which depends on the old AssetPaths api.

Whats the best way to update this stuff w/o making the build red? Maybe create a new branch on sprockets-rails that works w/ this pull?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 15, 2012

Member

We will need to update the sprockets-rails project to not use this API. cc @guilleiguaran

Member

rafaelfranca commented Oct 15, 2012

We will need to update the sprockets-rails project to not use this API. cc @guilleiguaran

@josh josh referenced this pull request Oct 15, 2012

Merged

Use new asset url helper apis #23

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Oct 15, 2012

Member

This needs rebase 馃榿

Member

guilleiguaran commented Oct 15, 2012

This needs rebase 馃榿

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Oct 15, 2012

Member

@josh btw, I think sass-rails is also depending in AssetsPath API

Member

guilleiguaran commented Oct 15, 2012

@josh btw, I think sass-rails is also depending in AssetsPath API

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Oct 15, 2012

Member

@guilleiguaran don't see any calls there. Also, you should be able to remove most of that plugin. The builtin sass stuff works pretty damn well.

Member

josh commented Oct 15, 2012

@guilleiguaran don't see any calls there. Also, you should be able to remove most of that plugin. The builtin sass stuff works pretty damn well.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh Oct 15, 2012

Member

Aight, railties tests finally finished here.

What order should the branches be merged?

Member

josh commented Oct 15, 2012

Aight, railties tests finally finished here.

What order should the branches be merged?

rafaelfranca added a commit that referenced this pull request Oct 15, 2012

@rafaelfranca rafaelfranca merged commit 046ab84 into rails:master Oct 15, 2012

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 15, 2012

Member

Merged. Thanks

Member

rafaelfranca commented Oct 15, 2012

Merged. Thanks

rafaelfranca added a commit that referenced this pull request Oct 18, 2012

Add CHANGELOG entry for #7927
Removed the asset_path configuration from the guides and added an entry
in the upgrading_ruby_on_rails guide

[ci skip]

rafaelfranca added a commit that referenced this pull request Oct 18, 2012

Add more CHANGELOG entries from #7927
Also remove references for RAILS_ASSET_ID from the guides

[ci skip]
@jejacks0n

This comment has been minimized.

Show comment
Hide comment
@jejacks0n

jejacks0n Jan 3, 2013

Contributor

Josh, just wanted to understand this commit -- I know it's from 3 months ago, but I'm seeing what appears to be a bug with rendering assets from views within an engine.. I'm using javascript_include_tag, and they get prepended with wherever that engine was mounted at within the rails app.

I would appreciate you taking a look at an issue I created in rails/sprockets-rails, and a project that exhibits the issue.

When an engine is mounted within an application using mount Foo::Engine => "/path" then request.try(:script_name) will return "/path" -- this results in the asset urls being incorrectly prepended with /path/assets.

Thanks.

EDIT: #8712

Contributor

jejacks0n commented on actionpack/lib/action_view/helpers/asset_url_helper.rb in e6451a5 Jan 3, 2013

Josh, just wanted to understand this commit -- I know it's from 3 months ago, but I'm seeing what appears to be a bug with rendering assets from views within an engine.. I'm using javascript_include_tag, and they get prepended with wherever that engine was mounted at within the rails app.

I would appreciate you taking a look at an issue I created in rails/sprockets-rails, and a project that exhibits the issue.

When an engine is mounted within an application using mount Foo::Engine => "/path" then request.try(:script_name) will return "/path" -- this results in the asset urls being incorrectly prepended with /path/assets.

Thanks.

EDIT: #8712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment