Skip to content

[Fix #95] Raise When Dependencies Improperly Used#96

Merged
rafaelfranca merged 1 commit intorails:masterfrom
schneems:schneems/raise-on-dependencies
Dec 12, 2013
Merged

[Fix #95] Raise When Dependencies Improperly Used#96
rafaelfranca merged 1 commit intorails:masterfrom
schneems:schneems/raise-on-dependencies

Conversation

@schneems
Copy link
Copy Markdown
Member

If a dependency is used in an ERB asset that references another asset, it will not be updated when the reference asset is updated. The fix is to use //= depend_on or its cousin //= depend_on_asset however this is easy to forget. See #95 for more information.

Currently Rails/Sprockets hides this problem, and only surfaces it when the app is deployed with precompilation to production multiple times. We know that you will have this problem if you are referencing assets from within other assets and not declaring them as dependencies. This PR checks if you've declared a given file as a dependency before including it via asset_path. If not a helpful error is raised:

Asset depends on 'bootstrap.js' to generate properly but has not declared the dependency
Please add: `//= depend_on_asset "bootstrap.js"` to '/Users/schneems/Documents/projects/codetriage/app/assets/javascripts/application.js.erb'

Implementation is quite simple and limited to helper.rb, additional code is all around tests.

ATP

If a dependency is used in an ERB asset that references another asset, it will not be updated when the reference asset is updated. The fix is to use `//= depend_on` or its cousin `//= depend_on_asset` however this is easy to forget. See rails#95 for more information.

Currently Rails/Sprockets hides this problem, and only surfaces it when the app is deployed with precompilation to production multiple times. We know that you will have this problem if you are referencing assets from within other assets and not declaring them as dependencies. This PR checks if you've declared a given file as a dependency before including it via `asset_path`. If not a helpful error is raised:

```
Asset depends on 'bootstrap.js' to generate properly but has not declared the dependency
Please add: `//= depend_on_asset "bootstrap.js"` to '/Users/schneems/Documents/projects/codetriage/app/assets/javascripts/application.js.erb'
```

Implementation is quite simple and limited to `helper.rb`, additional code is all around tests.

ATP
@rafaelfranca
Copy link
Copy Markdown
Member

Seems 👍 for me.

@guilleiguaran
Copy link
Copy Markdown
Member

/cc @josh

@bf4
Copy link
Copy Markdown

bf4 commented Dec 4, 2013

👍

2 similar comments
@jamesbrooks
Copy link
Copy Markdown

👍

@benzimmer
Copy link
Copy Markdown

👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def check_dependencies!(dep)
  if @_dependency_assets && !@_dependency_assets.detect { |asset| asset.include?(dep) }
    raise DependencyError.new(self.pathname, dep)
  end
end

To avoid the double return,

schneems added a commit to schneems/sprockets that referenced this pull request Dec 10, 2013
This PR exposes `matches_filter` via a public method called `filtered?` for the purposes of detecting if an asset exists in a filter. This is to be used in rails/sprockets-rails#96. This change to sprockets was requested by @rafaelfranca to prevent the interface from accidentally changing or breaking.
schneems added a commit to schneems/sprockets that referenced this pull request Dec 10, 2013
This PR exposes `matches_filter` via a public method called `filtered?` for the purposes of detecting if an asset exists in a filter. This is to be used in rails/sprockets-rails#96. This change to sprockets was requested by @rafaelfranca to prevent the interface from accidentally changing or breaking.
rafaelfranca pushed a commit that referenced this pull request Dec 12, 2013
[Fix #95] Raise When Dependencies Improperly Used
@rafaelfranca rafaelfranca merged commit f4ff87d into rails:master Dec 12, 2013
@bf4
Copy link
Copy Markdown

bf4 commented Dec 12, 2013

👍 (If only Github had a less verbose comment). Congrats!

@elia
Copy link
Copy Markdown

elia commented Dec 12, 2013

👍 🐸

@vipulnsward
Copy link
Copy Markdown
Member

This deserves a CHANGELOG. A lot of apps are going to start throwing exceptions now.

@jaredbeck
Copy link
Copy Markdown
Contributor

Has this been released yet? It looks like there hasn't been a release since 2.0.1 (October 16, 2013). Thanks.

@rafaelfranca
Copy link
Copy Markdown
Member

Not yet. I'll cook a new release until Monday

@bf4
Copy link
Copy Markdown

bf4 commented Apr 2, 2014

(bump)

@rafaelfranca
Copy link
Copy Markdown
Member

2.1.0 was released

@jaredbeck
Copy link
Copy Markdown
Contributor

@schneems should we still use sprockets_better_errors in our rails 4.0.x apps (< 4.1) or can we just use this new 2.1.0 release of sprockets-rails? Thanks! PS: I can give it a try later this week if that helps.

@schneems
Copy link
Copy Markdown
Member Author

schneems commented Apr 8, 2014

`sprockets_better_errors should be redundant. If you want to give me a PR
to limit the gem spec to < 4.1 I would appreciate it

On Sun, Apr 6, 2014 at 11:20 PM, Jared Beck notifications@github.comwrote:

@schneems https://github.com/schneems should we still use
sprockets_better_errorshttps://github.com/schneems/sprockets_better_errorsin our rails 4.0.x apps (< 4.1) or can we just use this new 2.1.0 release
of sprockets-rails? Thanks! PS: I can give it a try later this week if that
helps.

Reply to this email directly or view it on GitHubhttps://github.com//pull/96#issuecomment-39695102
.

bf4 added a commit to bf4/sprockets_better_errors that referenced this pull request Apr 8, 2014
rroblak pushed a commit to insight-meditation-center/imc-bootstrap-sass that referenced this pull request Nov 9, 2015
bluedone added a commit to bluedone/bootstrap-rubygem that referenced this pull request Jun 6, 2023
evanhughesdev added a commit to evanhughesdev/bootstrap-rubygem that referenced this pull request Mar 22, 2025
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.

9 participants